1169667 - DevTools: when toggling the "Highlight painted area" tool, the icon color is wrong

Status: VERIFIED FIXED

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

Description

[Jarda Snajdr [:jsnajdr]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150528030206

Steps to reproduce:

1. Open DevTools and enable the "Highlight painted area" tool.
2. Click on the "Highlight painted area" toolbar button.
3. The function is turned on and the icon gets blue.
4. Click again to turn highlighting off.


Actual results:

- highligthing is turned off, that's OK
- but the icon remains blue, as if the tool was still active
- the <toolbarbutton> still has attribute checked="true"


Expected results:

- icon is toggled back to disabled state, has black color
- the <toolbarbutton> is not in "checked" state

Comment 1

[Gingerbread Man]

I can reproduce this in FDE and Nightly, but not Beta 39.0 or Release 38.0.1. Disabling e10s in Nightly and restarting made no difference.

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
20150529030205
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
20150529004007

Comment 2

[Gingerbread Man]

2015-03-29: works properly.
https://hg.mozilla.org/mozilla-central/rev/385840329d91

2015-03-30 → 2015-04-23: with e10s, clicking the button does nothing besides flooding the Browser Console with errors; without e10s, it works properly.
https://hg.mozilla.org/mozilla-central/rev/6082a98d3861
https://hg.mozilla.org/mozilla-central/rev/0b202671c9e2

2015-04-24: the button now works with e10s again, but exhibits the bug described in this report, whether e10s is enabled or not. 
https://hg.mozilla.org/mozilla-central/rev/22a157f7feb7

Comment 3

[kud]

I've got the same problem on `41.0a1 (2015-06-02)` without e10s enabled.

Comment 4

[Jarda Snajdr [:jsnajdr]]

Working on this, please assign to me.

Comment 5

[Gingerbread Man]

(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> Working on this, please assign to me.

Thank you.

Comment 6

[Jarda Snajdr [:jsnajdr]]

Attachment: DevTools: Flashpainting icon has wrong color when toggled on/off

Here is a patch. The bug was caused by incorrect handling of paintflashing_server command output - an Output object was treated as boolean.

Added a test that check the correct state.isChecked() status after every change.

Comment 7

[Patrick Brosset <:pbro>]

Reminder to push to TRY when the tree re-opens.

Comment 8

[Patrick Brosset <:pbro>]

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=792d8f928cb7

Comment 9

[Jarda Snajdr [:jsnajdr]]

The test browser/devtools/shared/test/browser_telemetry_button_paintflashing.js fails after my patch.

The DEVTOOLS_PAINTFLASHING_TIME_ACTIVE_SECONDS is an array that should have 2 entries, each containing for how long the tool has been active. Because the toggle button has been clicked 4 times.

However, it contains only one record, because the check happens immediately after calling the last click(), without waiting for the async action to be finished. The ACTIVE_SECONDS record is added only after the tool is closed and the active time can be computed.

Before my patch, the telemetry results didn't contain any ACTIVE_SECONDS record at all (which the test fails to notice), so no test on it was really performed. That was caused by the bug being fixed - the paintflashing tool always reported its state as "opened". The computation of active time never starts.

To fix:
- instead of calling setTimeout, we should wait for some event telling us that the click action was finished and we can check the telemetry results. But which one?
- test that ACTIVE_SECONDS record is always present.
- it's very suspicious that method checkResults is called before stopRecordingTelemetryLogs. We should check after the recording is finished.

Comment 10

[Jarda Snajdr [:jsnajdr]]

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

Comment 11

[Jarda Snajdr [:jsnajdr]]

Attachment: DevTools: Flashpainting icon has wrong color when toggled on/off

Fixed the browser_telemetry_button_paintflashing.js test:
- wait for the CLI command to be finished before doing anything else - using a commandOutputManager as used also in browser_telemetry_button_eyedropper.js
- rewrite the delayedClicks function into a generator function - much better code without nested promises

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6ed69748707

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1eb9c47baaf9

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/1eb9c47baaf9

Comment 14

[Nazir Ahmed Sabbir [:NaSb]]

I found this issue in Nightly 41.0a1 (2015-05-29) (Build ID: 20150529030205) on Linux x64

This Bug is now Verified as fixed on Latest Firefox Beta 41.0b3

Build ID : 20150820142145
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0

Comment 15

[Forhad Hossain]

I have reproduced this bug in Nightly 41.0a1 (2015-05-29)(Build ID:20150529030205) with Comment 0's instruction on Windows 10 64bit.

Bug is fixed now on latest Beta 41.0b3 (Build ID:20150820142145)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0

[testday-20150821]

Comment 16

[Nazir Ahmed Sabbir [:NaSb]]

As it is verified on Linux and Windows (Comment 14 and Comment 15), Marking it as verified!

Comment 17

[arni2033]

This is regression from bug 1128988 (between 2015-04-23 and 2015-04-24),
and it wasn't completely fixed (see bug 1209149). Regression range:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b202671c9e2&tochange=22a157f7feb7