Closed Bug 848760 Opened 7 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1163332

People

(Reporter: paul, Unassigned)

Details

(Whiteboard: [waiting for green try])

Attachments

(2 files, 2 obsolete files)

Attachment #775134 - Flags: review?(paul)
Comment on attachment 775134 [details] [diff] [review]
Add screenshot menu item to inspector

Looks good to me! Thanks.

Just rename panelWindow to browserWindow.
Attachment #775134 - Flags: review?(paul) → review+
Blocks: 849236
I am just concerned on having three different implementations of an almost similar approach to capture screenshots.
(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.
(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.
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).
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #775134 - Attachment is obsolete: true
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.
Whiteboard: [waiting for green try]
erf, in the test:

s/setInterval/setTimeout/
Apparently, the test (or the feature) doesn't work on Windows XP.
Attached patch patch v1.2Splinter Review
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.
Attachment #775888 - Attachment is obsolete: true
Attached patch over actorSplinter Review
Because the inspector now works as a remote tool, I needed to move this code at the actor level.
No longer blocks: 849236
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1163332
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.