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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Removed logging

Comment 4

[Tim Nguyen :ntim]

Attachment: Patch v1.2

Forgot to `hg add` the test.

Comment 5

[Tim Nguyen :ntim]

Attachment: Patch v1.3

Updated test description

Comment 6

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05fae4faeec2

Comment 7

[Tim Nguyen :ntim]

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

[Brian Grinstead [:bgrins]]

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.

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch v1.4

Comment 10

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03374f7aa64

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8616844 [details] [diff] [review]
Patch v1.4

Review of attachment 8616844 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM if the try push is green

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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.

Comment 13

[Tim Nguyen :ntim]

(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 ?

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

(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.

Comment 15

[Tim Nguyen :ntim]

(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 16

[Tim Nguyen :ntim]

Re-pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1cf51b7250d

Comment 17

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe5956af2de0

Comment 18

[Tim Nguyen :ntim]

Try is greeen !

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4bd79939fea1

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4bd79939fea1