Closed Bug 1155402 Opened 5 years ago Closed 4 years ago

Standalone client needs new prompt for Chrome 44

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
1

Tracking

(firefox42 verified)

VERIFIED FIXED
mozilla42
Tracking Status
firefox42 --- verified

People

(Reporter: abr, Assigned: c0mrad3, Mentored)

Details

(Whiteboard: [UX bug - Chrome impact now] [good first bug] [lang=css,js])

Attachments

(3 files, 1 obsolete file)

It would appear that we need a new camera prompt for Chrome starting with version 44: they've gone from a ribbon to a doorhanger for camera/mic permissions.

See attached screenshot for an example.
Attached image Chrome2.svg
Attached, find the new prompt graphic provided by Sevaan.
determine when chrome 44 is releasing to make sure we pull it for then.  at 42 currently - not sure on cycles.
Rank: 31
Flags: needinfo?(sescalante)
Flags: firefox-backlog+
Priority: -- → P3
chrome 44 is live right now - so need to prioritize this one.

is this a good first bug?
Points: --- → 1
Rank: 31 → 23
Flags: needinfo?(sescalante) → needinfo?(mdeboer)
Priority: P3 → P2
Whiteboard: [UX bug - Chrome impact now]
Mentor: mdeboer, standard8
Flags: needinfo?(mdeboer)
Whiteboard: [UX bug - Chrome impact now] → [UX bug - Chrome impact now] [good first bug] [lang=css,js]
Flags: qe-verify+
Hey I would like to work on this bug please assign it to me :)

I think I figured out the most difficult part of it :) I just need to replace the current gum-chrome.svg with the one attached by you in the comment 2 

at the path mozilla-central/browser/components/loop/standalone/content/img 

only thing that I am left with is to learn how to submit the patch :)
Flags: needinfo?(mdeboer)
I`ve added Thummala as assignee of this bug since he requested this on #irc and he does not have the privileges to do so.
Status: NEW → ASSIGNED
changed the gum-chrome.svg file to the one attached in the bugzilla attachment 2 [details] [diff] [review]

Hope you guys review the patch quickly and apply it :)
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #5)
> I`ve added Thummala as assignee of this bug since he requested this on #irc
> and he does not have the privileges to do so.

Hi Bogdan Maris,
Should I go create a PR for tryserver? just tell me what are the tests required.


Hi Dhanvi,
The patch looks good, just a small heads up for you for the next time onwards, 

1) When you include the commit message to the patch, include "r= mentor_name". That is the standard format of a patch. :) 

2) Everytime you create a patch, just give a review to the mentor so that he can verify the patch.
Mike's currently away from work; switching the review to Standard8 who will be back (though probably a bit swamped) on Monday.
Flags: needinfo?(mdeboer) → needinfo?(standard8)
I have to edit the patch to match the file format of others in the same directory 

Is it necessary @standard8
Attached patch bug-1155402-v2Splinter Review
I have submitted the final patch please take
Attachment #8641549 - Attachment is obsolete: true
Attachment #8641549 - Flags: review?(standard8)
Attachment #8641549 - Attachment is obsolete: false
I was a bit worried about needing to cope with older versions of Chrome. However, I took a look at our stats and the majority of the hits we got over the last week or so are chrome 44. Hence I think we can keep it simple and just replace the image directly - rather than having a before & after.
Flags: needinfo?(standard8)
Comment on attachment 8641549 [details] [diff] [review]
bug-1155402.patch changed the gum-chrome.svg file to the one attached in the bugzilla

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

We should go with the other patch that has the license with it.
Attachment #8641549 - Flags: review?(standard8) → review-
Attachment #8641549 - Attachment is obsolete: true
Comment on attachment 8642502 [details] [diff] [review]
bug-1155402-v2

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

Looks great. Thanks for the patch, there's a couple of minor amendments, but since they are minor I'll fix them for you on check-in which I'll do in a couple of minutes.

> Bug 1155402 - Change standalone prompt for Chrome 44

My suggest would be something like "Bug 1155402 - Change Loop's standalone prompt for gUM to align with Chrome 44 changes.". Its a little bit more descriptive, includes the "Loop" word so that non people can identify it as part of Loop/Hello without looking into the details, and includes a bit more detail as to what we're changing.

::: browser/components/loop/standalone/content/img/gum-chrome.svg
@@ +1,5 @@
>  <?xml version="1.0" encoding="UTF-8" standalone="no"?>
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="101px" height="62px" viewBox="0 0 101 62" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

This svg element still needs the ' xmlns:sketch="http://www.bohemiancoding.com/sketch/ns"' within it otherwise it isn't readable.
Attachment #8642502 - Flags: review+
Ok, this has now landed in our integration branch, it'll be merged to the main mozilla-central repository sometime in the next 24 hours or so (at which time this bug will automatically be marked as fixed)

The new image will be live the next time we do a standalone release, which is likely to be in about 2 weeks.

Thanks again for the patch.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Mark Banner (:standard8) from comment #13)
> Comment on attachment 8642502 [details] [diff] [review]
> bug-1155402-v2
> 
> Review of attachment 8642502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great. Thanks for the patch, there's a couple of minor amendments, but
> since they are minor I'll fix them for you on check-in which I'll do in a
> couple of minutes.
> 
> > Bug 1155402 - Change standalone prompt for Chrome 44
> 
> My suggest would be something like "Bug 1155402 - Change Loop's standalone
> prompt for gUM to align with Chrome 44 changes.". Its a little bit more
> descriptive, includes the "Loop" word so that non people can identify it as
> part of Loop/Hello without looking into the details, and includes a bit more
> detail as to what we're changing.
I will keep that In mind :) 
> 
> ::: browser/components/loop/standalone/content/img/gum-chrome.svg
> @@ +1,5 @@
> >  <?xml version="1.0" encoding="UTF-8" standalone="no"?>
> >  <!-- This Source Code Form is subject to the terms of the Mozilla Public
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg width="101px" height="62px" viewBox="0 0 101 62" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> 
> This svg element still needs the '
> xmlns:sketch="http://www.bohemiancoding.com/sketch/ns"' within it otherwise
> it isn't readable.

I thought It was just some branding as I was able to view the image without the line in my LXDE image viewer
(In reply to Mark Banner (:standard8) from comment #15)
> Ok, this has now landed in our integration branch, it'll be merged to the
> main mozilla-central repository sometime in the next 24 hours or so (at
> which time this bug will automatically be marked as fixed)
> 
> The new image will be live the next time we do a standalone release, which
> is likely to be in about 2 weeks.
Thanks for the heads up 
> 
> Thanks again for the patch.

:) cool more patches coming for some other components
https://hg.mozilla.org/mozilla-central/rev/e0973a29cb1c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Verified as fixed making a call between Firefox 42beta3 and Chrome 45.0.2454.101 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.