1028234 - Allow command buttons to use target to define the current target rather than tab

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

The event to update the state of a command button currently requires a tab as the event argument. This will break with e10s, also 'target' is the right thing to do here.

Comment 1

[Joe Walker [:jwalker]]

This bug will not attempt to remove all places where 'tab' is used. That will be for a follow-up bug.

Comment 2

[Joe Walker [:jwalker]]

Attachment: 0002-Bug-1028234-Allow-command-buttons-to-use-target-r-bg.patch

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8443545 [details] [diff] [review]
0002-Bug-1028234-Allow-command-buttons-to-use-target-r-bg.patch

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

Looks good to me.  Maybe some tests will need to change now if they were checking for the 'checked' attribute on the button immediately - worth a try push

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +133,5 @@
> +           * a tab property.
> +           */
> +          let onChange = (eventName, ev) => {
> +            if (ev.target == target || ev.tab == target.tab) {
> +              let reply = command.state.isChecked(target);

So I presume the return type of isChecked is going to change to a promise in the future as each command becomes e10s compliant?  Not sure if this interface is documented anywhere, but it should be updated if so

Comment 4

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8443545 [details] [diff] [review]
> 0002-Bug-1028234-Allow-command-buttons-to-use-target-r-bg.patch
> 
> Review of attachment 8443545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.  Maybe some tests will need to change now if they were
> checking for the 'checked' attribute on the button immediately - worth a try
> push
> 
> ::: browser/devtools/shared/DeveloperToolbar.jsm
> @@ +133,5 @@
> > +           * a tab property.
> > +           */
> > +          let onChange = (eventName, ev) => {
> > +            if (ev.target == target || ev.tab == target.tab) {
> > +              let reply = command.state.isChecked(target);
> 
> So I presume the return type of isChecked is going to change to a promise in
> the future as each command becomes e10s compliant?  Not sure if this
> interface is documented anywhere, but it should be updated if so

The problem is very annoying. TargetFactory.forTab returns a target that might not be remoted. The developer toolbar executes against the current tab, so any keypress in GCLI might be against an unremoted target. This is a symptom of this problem. E10S or merging GCLI into the toolbox will fix this, but I want to avoid patches exploding too much, so this is a hack.

I'll post an update soon that avoids breaking webconsole tests.

Comment 5

[Joe Walker [:jwalker]]

Attachment: 0002-Bug-1028234-Allow-command-buttons-to-use-target-r-bg.patch

This differs from the previous patch in that onChange in DeveloperToolbar.jsm now checks to see if the reply from isChecked is a promise so it can deal with non-promises synchronously.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8444374 [details] [diff] [review]
0002-Bug-1028234-Allow-command-buttons-to-use-target-r-bg.patch

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

Looks fine to me with a green try push

Comment 7

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=d8d6fce40dbd

Comment 8

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=5def913879a7
https://hg.mozilla.org/integration/fx-team/rev/5def913879a7

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/5def913879a7