[app manager] move the screenshot button in the footer

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: paul, Assigned: Marvin Sevilla)

Tracking

Trunk
Firefox 29
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=html][lang=js][mentor=paul])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 825253 [details]
screenshot

Comment 1

4 years ago
Hi, 
   
  I am willing to fix this bug.So, please I request you to assign me this bug.

Thanks,

Regards,
Gaurav Saini
(Reporter)

Comment 2

4 years ago
(In reply to Gaurav Saini from comment #1)
> I am willing to fix this bug.So, please I request you to assign me this bug.

Sure. Code lives there: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/

Original button is here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/content/device.xhtml#39

We want to move it there: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/content/connection-footer.xhtml

Updated

4 years ago
Whiteboard: [good-first-bug][lang=html][lang=js][mentor=paul] → [good first bug][lang=html][lang=js][mentor=paul]
Hello, is Gaurav Saini still working on this?

Comment 4

4 years ago
(In reply to Peiyong LIn from comment #3)
> Hello, is Gaurav Saini still working on this?

Yes, I am still working on this.
Hi, 

I created a patch for this. I put the button in the footer, but i have a problem, the button didn't work. I was trying to add at the final:

<script type="application/javascript;version=1.8" src="device.js"></script>

in connection-footer.xhtml, but it didn't work. I enabled the Browser console, i can't see a warning/alert related to this. If i don't add the device.js file i got:

UI.screenshot: is not a defined function

I was trying to put the pice of code about screenshots from device.js to connection-footer.js but i didn't work. If you can give some tips and advices about what is wrong, i will really appreciate. I am going to attach the first version of the patch, if someone is interested on review it and tell me if everything is good to continue, i am going to be very happy.

Regards,
Gio
Created attachment 833390 [details] [diff] [review]
name.patch
(Reporter)

Comment 7

4 years ago
> I was trying to put the pice of code about screenshots from device.js to connection-footer.js but i didn't work.

That's the right way to do it. What didn't work?
It didn't open a new tab and take the screenshot. Look this patch with the modifications.
Created attachment 8333682 [details] [diff] [review]
fix.patch
Attachment #8333682 - Flags: review?(paul)
(Reporter)

Comment 10

4 years ago
Comment on attachment 8333682 [details] [diff] [review]
fix.patch

You need to adapt the code to make it work from connection-footer.js. Here this.connected doesn't exist, getDeviceFront either. Look at the browser console, you'll see plenty of errors.
Attachment #8333682 - Flags: review?(paul)
(Assignee)

Comment 11

4 years ago
Hello, is Gaurav Saini working on this? If not, I would like to be assigned to this bug please.
(Assignee)

Comment 12

4 years ago
Sorry, I mean is Giovanny working on this? If not, I would like to be assigned to this bug please.

Updated

4 years ago
Flags: needinfo?(gioyik)
So, i would like to work on this bug, anyone who can assign me this one.

thank you

Regards

Meghraj Suthar
(Assignee)

Comment 14

4 years ago
Created attachment 8367033 [details] [diff] [review]
rev1 - App manager moved screenshot button

I've attached my proposed patch. Let me know if this looks okay, thanks!
Attachment #8367033 - Flags: review?(paul)
(Reporter)

Comment 15

4 years ago
Comment on attachment 8367033 [details] [diff] [review]
rev1 - App manager moved screenshot button

That's exactly what we need.
Can I just ask you to swap disconnect and screenshot? (disconnect first, then screenshot). Then we'll land your patch.

Thanks a lot!
Attachment #8367033 - Flags: review?(paul) → review+
Flags: needinfo?(gioyik)
(Reporter)

Updated

4 years ago
Assignee: nobody → agentmirv
(Assignee)

Comment 16

4 years ago
Created attachment 8367228 [details] [diff] [review]
rev2 - App manager moved screenshot button

I have attached my revised patch. I swapped the disconnect and screenshot buttons (disconnect first, then screenshot). Thanks!
Attachment #8367033 - Attachment is obsolete: true
Attachment #8367228 - Flags: review?(paul)
(Reporter)

Updated

4 years ago
Attachment #833390 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8333682 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8367228 - Flags: review?(paul) → review+
(Reporter)

Comment 17

4 years ago
Thank you Marvin!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/204e3e268330
Keywords: checkin-needed
Whiteboard: [good first bug][lang=html][lang=js][mentor=paul] → [good first bug][lang=html][lang=js][mentor=paul][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/204e3e268330
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=html][lang=js][mentor=paul][fixed-in-fx-team] → [good first bug][lang=html][lang=js][mentor=paul]
Target Milestone: --- → Firefox 29
Keywords: dev-doc-needed
App Manager is replaced by WebIDE, so I'm clearing this dev-doc-needed.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.