J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Description • 8 years ago

STR:

1. Add custom device, enable it, select it
2. Check that UA is applied from device
3. Remove custom device

ER:

UA is reset to default

AR:

UA from custom device still applied

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 2 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ)
	Comment 3 • 8 years ago
mozreview-review

Comment on attachment 8836222 [details]
Bug 1338604 - Tell manager when custom devices are removed.

https://reviewboard.mozilla.org/r/111686/#review113392

::: devtools/client/responsive.html/app.js:114
(Diff revision 1)
>    onRemoveCustomDevice(device) {
>      this.props.dispatch(removeCustomDevice(device));
>    },
>  
>    onRemoveDeviceAssociation(id) {
>      // TODO: Bug 1332754: Move messaging and logic into the action creator.

Should we remove this TODO since the messaging logic got removed here?

::: devtools/client/responsive.html/utils/message.js:26
(Diff revision 1)
>    win.addEventListener("message", onMessage);
>  
>    return deferred.promise;
>  }
>  
> -function post(win, type) {
> +function post(win, typeOrMessage) {

It is probably a good idea to start adding JSDocs and defining the @param {type} since it is starting to get confusing what type of params are expected for this function.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 4 • 8 years ago
mozreview-review-reply

Comment on attachment 8836222 [details]
Bug 1338604 - Tell manager when custom devices are removed.

https://reviewboard.mozilla.org/r/111686/#review113392

> Should we remove this TODO since the messaging logic got removed here?

I'll tweak the comment slightly, as I think I'd still like the device property changes to move to the action creator.

> It is probably a good idea to start adding JSDocs and defining the @param {type} since it is starting to get confusing what type of params are expected for this function.

Good idea, added some docs.

	Pulsebot
	Comment 6 • 8 years ago

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3dab544f6b6a
Tell manager when custom devices are removed. r=gl

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 7 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3dab544f6b6a

	ovidiu boca[:Ovidiu]
	Comment 8 • 8 years ago

Hi Ryan,

I tried to test this on Mac, Ubuntu, Windows 10 and I still can reproduce this, UA from custom device still applied. 
I attached a video when I reproduce this issue. Please tell me if this are the actions from comment 0 ?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 9 • 8 years ago

(In reply to ovidiu boca[:Ovidiu] from comment #8)
> Hi Ryan,
> 
> I tried to test this on Mac, Ubuntu, Windows 10 and I still can reproduce
> this, UA from custom device still applied. 
> I attached a video when I reproduce this issue. Please tell me if this are
> the actions from comment 0 ?

Hmm, where is the video?  I think I would need more detail about the steps you tried and / or the video.

	ovidiu boca[:Ovidiu]
	Comment 10 • 8 years ago

Sorry for not attaching the video, I think I forgot. Here it is another video with my actions. Please tell me if what I do in the video is correct and if not please correct me. Thanks

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 11 • 8 years ago

(In reply to ovidiu boca[:Ovidiu] from comment #10)
> Created attachment 8837969 [details]
> Recording #3.mp4
> 
> Sorry for not attaching the video, I think I forgot. Here it is another
> video with my actions. Please tell me if what I do in the video is correct
> and if not please correct me. Thanks

It looks like in your video, you are pointing out that the size is left as is after deleting the device.  This behavior seems fine to me.

With your custom device in the video, the user agent / UA field was left with the default value.  Here's what I would expect to see:

1. Open RDM
2. Open the console (Developer -> Web Console)
3. Check the user agent value with "navigator.userAgent" in the console (should give a default value similar to "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0")
4. Add custom device with some custom UA like "iPhone 9000"
5. Enable it in device modal
6. Select it in the device selector
7. Check the user agent value with "navigator.userAgent" in the console (should give the custom "iPhone 9000" value)
8. Remove custom device
9. Check the user agent value with "navigator.userAgent" in the console (should go back to default value from step 3)

Other device properties like DPR should also be reset when removing the custom device.  The size, however, will be left alone.

	ovidiu boca[:Ovidiu]
	Comment 12 • 8 years ago

Hi,

I tested following your steps from comment 11 on Mac OS X 10.12, Windows 10, Ubuntu 16.04 with Nightly 54.0a1 (2017-02-16) and I can confirm the fix. 

I will mark this bug as Verified: Fix. 

Also I saw a problem, here are the steps:

1. Add a device
2. After the device is added in the Custom list, hover the mouse over the "X" 

Please see the attached screen shot, tell me if I need to fill in another bug for this. Thanks