Closed
Bug 1165032
Opened 9 years ago
Closed 9 years ago
Make "add new rule" button add a new rule for the currently emulated pseudo class lock(s)
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: ntim, Assigned: ntim)
References
Details
Attachments
(1 file, 4 obsolete files)
7.90 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Sorry mid-air collision when cc'ing myself, but I don't think this should block pseudo class locks panel since this only affects the Add Rule functionality.
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Flags: qe-verify+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Updated•9 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8616393 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 3•9 years ago
|
||
Removed logging
Attachment #8616393 -
Attachment is obsolete: true
Attachment #8616393 -
Flags: review?(bgrinstead)
Attachment #8616394 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 4•9 years ago
|
||
Forgot to `hg add` the test.
Attachment #8616394 -
Attachment is obsolete: true
Attachment #8616394 -
Flags: review?(bgrinstead)
Attachment #8616395 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 5•9 years ago
|
||
Updated test description
Attachment #8616395 -
Attachment is obsolete: true
Attachment #8616395 -
Flags: review?(bgrinstead)
Attachment #8616397 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05fae4faeec2
Assignee | ||
Comment 7•9 years ago
|
||
This seems backwards compatible with older servers. I have tried adding a new rule on an older server, worked fine. I also did the backwards procedure (adding a new rule from an older server), worked fine as well.
Comment 8•9 years ago
|
||
Comment on attachment 8616397 [details] [diff] [review] Patch v1.3 Review of attachment 8616397 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good overall - just a few notes ::: browser/devtools/styleinspector/test/browser_ruleview_add-rule_pseudo_class.js @@ +91,5 @@ > + !view.activeCheckbox.checked && > + !view.focusCheckbox.checked) { > + return; > + } > + let onRefresh = inspector.once("rule-view-refreshed"); Could this be a race condition waiting to happen? If there are multiple locks that need to be removed then in theory this event could fire for the first one but not quite be done for the second/third when the function returns. Maybe we should be scoping onRefresh and the yield inside of each of these if (view.fooCheckbox.checked) blocks. ::: toolkit/devtools/server/actors/styles.js @@ +862,5 @@ > > /** > * Adds a new rule, and returns the new StyleRuleActor. > * @param NodeActor node > + * @param Array of pseudo classes The type in this comment could be [string] @param [string] pseudoClasses The list of pseudo classes to append to the new selector. @@ +890,5 @@ > return this.getNewAppliedProps(node, cssRules.item(index)); > }, { > request: { > + node: Arg(0, "domnode"), > + pseudoclasses: Arg(1, "array:string") This should be "nullable:array:string" so the request doesn't fail if no argument is passed in.
Attachment #8616397 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8616397 -
Attachment is obsolete: true
Attachment #8616844 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03374f7aa64
Comment 11•9 years ago
|
||
Comment on attachment 8616844 [details] [diff] [review] Patch v1.4 Review of attachment 8616844 [details] [diff] [review]: ----------------------------------------------------------------- LGTM if the try push is green
Attachment #8616844 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
You may want to re-push to try. There were a bunch of infra issues and it looks like it's preventing some of the tests on your try push from ever finishing, and I'm not sure if they can be re-triggered from the messed up state.
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #12) > You may want to re-push to try. There were a bunch of infra issues and it > looks like it's preventing some of the tests on your try push from ever > finishing, and I'm not sure if they can be re-triggered from the messed up > state. The Windows and OSX try pushes are passing. Is that enough or should I push again to try ?
Flags: needinfo?(wkocher)
Comment 14•9 years ago
|
||
(In reply to Tim Nguyen [:ntim] from comment #13) > (In reply to Wes Kocher (:KWierso) from comment #12) > > You may want to re-push to try. There were a bunch of infra issues and it > > looks like it's preventing some of the tests on your try push from ever > > finishing, and I'm not sure if they can be re-triggered from the messed up > > state. > > The Windows and OSX try pushes are passing. Is that enough or should I push > again to try ? I think you should push again since not all the tests are actually running from the looks of it.
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Gabriel Luong [:gl] from comment #14) > (In reply to Tim Nguyen [:ntim] from comment #13) > > (In reply to Wes Kocher (:KWierso) from comment #12) > > > You may want to re-push to try. There were a bunch of infra issues and it > > > looks like it's preventing some of the tests on your try push from ever > > > finishing, and I'm not sure if they can be re-triggered from the messed up > > > state. > > > > The Windows and OSX try pushes are passing. Is that enough or should I push > > again to try ? > > I think you should push again since not all the tests are actually running > from the looks of it. All the tests *did* run, see http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/d29af331f65b9aaf9fedab1c8992823b3cafcafcfe50a999fa307820d98c63873697ab1f714f048bd0b1e7b388b651860719c2db001f2f9ef5e186580043a272
Comment hidden (obsolete) |
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe5956af2de0
Assignee | ||
Comment 18•9 years ago
|
||
Try is greeen !
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4bd79939fea1
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/4bd79939fea1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•