Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Fix default bucketing#2897

Merged
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
jasonLaster:fix-default-bucketing
May 15, 2017
Merged

Fix default bucketing#2897
jasonLaster merged 3 commits into
firefox-devtools:masterfrom
jasonLaster:fix-default-bucketing

Conversation

@jasonLaster
Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster commented May 15, 2017

Associated Issue: #2857

Summary of Changes

We are currently showing [default properties] incorrectly when an object happens to have a window property like location.

This fixes that by explicitly checking to see if the object is a window.

Test Plan

  • adds unit tests
  • adds storybook stories

Screenshots/Videos (OPTIONAL)

@jasonLaster jasonLaster force-pushed the fix-default-bucketing branch 2 times, most recently from 284788c to 9c70ae2 Compare May 15, 2017 13:22
createNode("[default properties]", `${parentPath}/default`, defaultNodes)
createNode(
"[default properties]",
`${parentPath}/##-default`,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to what we do with symbol paths below.

@jasonLaster
Copy link
Copy Markdown
Contributor Author

one mochitest error:

6874 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-scopes.js | undefined assertion name - Got length, expected prototype

Copy link
Copy Markdown
Contributor

@codehag codehag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment regarding naming, otherwise good to go

Comment thread src/utils/object-inspector.js Outdated
ownProperties
);
} else {
} else if (objProps.class == "Window") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

expect(paths).to.eql(["root/bar", "root/__proto__"]);
});

it("window object", () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets give this a clearer descriptions... something like Sets default properties for the window object?

@jasonLaster jasonLaster force-pushed the fix-default-bucketing branch from 9c70ae2 to c1671f6 Compare May 15, 2017 18:39
@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2017

Codecov Report

Merging #2897 into master will increase coverage by 0.08%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2897      +/-   ##
==========================================
+ Coverage      60%   60.08%   +0.08%     
==========================================
  Files          63       63              
  Lines        2430     2435       +5     
  Branches      496      497       +1     
==========================================
+ Hits         1458     1463       +5     
  Misses        972      972
Impacted Files Coverage Δ
src/utils/object-inspector.js 62.83% <90%> (+1.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8b5f3...2d0a5bc. Read the comment docs.

@jasonLaster jasonLaster merged commit 4dcf905 into firefox-devtools:master May 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants