848760 - Add a "screenshot" menuitem to the inspector (in the highlighter infobar)

Status: RESOLVED DUPLICATE of bug 1163332

(DevTools :: Inspector, defect)

Description

[Paul Rouget [:paul]]

Here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#33

Comment 1

[Minarto Margoliono]

Attachment: Add screenshot menu item to inspector

Comment 2

[Paul Rouget [:paul]]

Comment on attachment 775134 [details] [diff] [review]
Add screenshot menu item to inspector

Looks good to me! Thanks.

Just rename panelWindow to browserWindow.

Comment 3

[Girish Sharma [:Optimizer]]

I am just concerned on having three different implementations of an almost similar approach to capture screenshots.

Comment 4

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #3)
> I am just concerned on having three different implementations of an almost
> similar approach to capture screenshots.

The one from gcli is much more complex (because it does more things) and barely reachable. I will rebase bug 849236 on this, and move the screenshot function to LayoutHelpers.

Comment 5

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #4)
> The one from gcli is much more complex (because it does more things) and
> barely reachable. I will rebase bug 849236 on this, and move the screenshot
> function to LayoutHelpers.

Great! Sounds reasonable.

I know that the one in gcli is much complex, but that will be just appropriate in this case, where we want to take a screenshot of a dom element (as that one already does that).
Although, it would have been nice if the method used in gcli could be moved to layout helpers and used everywhere.

Comment 6

[Minarto Margoliono]

Should the method in GCLI splitted into two at this point (move the first part to layout helper)? 

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/BuiltinCommands.jsm#1713


Maybe another one to generate the auto generated filename as well (If they should use the same name pattern).

Comment 7

[Paul Rouget [:paul]]

Let's keep things simple and land this patch. I will move things to LayoutHelpers later. Please rename the variable and I'll land that.

Comment 8

[Minarto Margoliono]

Attachment: patch v1.1

Comment 9

[Paul Rouget [:paul]]

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

Comment 10

[Paul Rouget [:paul]]

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

Comment 11

[Paul Rouget [:paul]]

Apparently, there's a test failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=25324149&tree=Try

Comment 12

[Paul Rouget [:paul]]

Not sure why it failed here... I might have pushed the wrong patch, and the previous push is not accessible anymore. I'll re-push to try.

Comment 13

[Paul Rouget [:paul]]

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

Comment 14

[Paul Rouget [:paul]]

erf, in the test:

s/setInterval/setTimeout/

Comment 15

[Paul Rouget [:paul]]

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

Comment 16

[Paul Rouget [:paul]]

Apparently, the test (or the feature) doesn't work on Windows XP.

Comment 17

[Minarto Margoliono]

Attachment: patch v1.2

Updated the test case to use setTimeout. 

I tried building and testing in XP. Even though the screenshot is saved to Download folder, the test case treat DfltDwnId as \Desktop. If I copied the screenshot to desktop before running the test case, the test will success and the desktop file is deleted.

Comment 18

[Paul Rouget [:paul]]

Attachment: over actor

Because the inspector now works as a remote tool, I needed to move this code at the actor level.