Make "add new rule" button add a new rule for the currently emulated pseudo class lock(s)

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

unspecified
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
No longer blocks: 987365
Depends on: 987365
Blocks: 987365
No longer depends on: 987365
(Assignee)

Updated

4 years ago
Blocks: 1136101
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

4 years ago
No longer blocks: 987365
See Also: → 987365
Flags: qe-verify+
(Assignee)

Updated

4 years ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Flags: qe-verify+
(Assignee)

Comment 2

4 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8616393 - Flags: review?(bgrinstead)
(Assignee)

Comment 3

4 years ago
Posted patch Patch v1.1 (obsolete) — Splinter Review
Removed logging
Attachment #8616393 - Attachment is obsolete: true
Attachment #8616393 - Flags: review?(bgrinstead)
Attachment #8616394 - Flags: review?(bgrinstead)
(Assignee)

Comment 4

4 years ago
Posted patch Patch v1.2 (obsolete) — Splinter Review
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

4 years ago
Posted patch Patch v1.3 (obsolete) — Splinter Review
Updated test description
Attachment #8616395 - Attachment is obsolete: true
Attachment #8616395 - Flags: review?(bgrinstead)
Attachment #8616397 - Flags: review?(bgrinstead)
(Assignee)

Comment 7

4 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 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

4 years ago
Posted patch Patch v1.4Splinter Review
Attachment #8616397 - Attachment is obsolete: true
Attachment #8616844 - Flags: review?(bgrinstead)
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

4 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

4 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)
(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

4 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 18

4 years ago
Try is greeen !
(Assignee)

Updated

4 years ago
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/4bd79939fea1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.