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

Once we're confident that the RDM redesign is performing well (probably a few cycles after 52?), we should remove the old RDM code.

Notes:

* Firefox OS desktop builds use the old RDM, but this may not matter anymore since it's being removed from the codebase in other ways already
* responsivedesign.jsm currently switches between the two paths, so we'll need to update integration points that call this.  Perhaps leave a small JSM around for Firefox UI code?  Unsure exactly.
* The responsivedesign-client.js frame script is also used by the new RDM, so it should be moved there
* Remove devtools/client/responsivedesign
* Possibly rename devtools/client/responsive.html to devtools/client/responsive assuming it is done in a way that preserves history

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

browser_styleeditor_media_sidebar_links.js was changed in bug 1296498 to support old and new RDM APIs.  The path for old RDM can be removed here.

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

New RDM is only used in e10s mode, and old RDM is used as the fallback if e10s is off.  So, we should keep old RDM around until e10s is the default everywhere.

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

Now that e10s has become the default, it's likely a good time to do this.

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

:bgrins notes a few more steps in bug 1403360:

* Remove the devtools.responsive.html.enabled pref
* Update / remove MDN docs - https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode_(before_Firefox_52), and the banner at https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=295e6a8a77cec0d3a921b2f56c1a18e549d54c45

	Alexandre Poirot [:ochameau]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8912458 [details]
Bug 1305777 - Move RDM frame script to new RDM.

https://reviewboard.mozilla.org/r/183780/#review189200

Thanks, it seems to look good. I tried to manual test everything and haven't seen any glitch.

Should we also remove these files as well?
  devtools/client/themes/responsivedesign.css
  devtools/client/locales/en-US/responsiveUI.properties

And also: devtools/shared/touch/simulator.js and devtools/shared/touch/simulator-content.js
It looks like only devtools/shared/touch/simulator-core.js is used by EmulationActor (which is the way to do touch for responsive.html).

I got this exception, but can't easily reproduce:
Full message: TypeError: this.emulationFront is undefined
Full stack: ResponsiveUI.prototype.updateTouchSimulation<@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:558:7
_run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
asyncFunction@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:246:14
onChangeTouchSimulation@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:499:5
handleMessage@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:462:9
handleEvent@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:434:9
EventListener.handleEvent*ResponsiveUI.prototype.init</this.swap<.getInnerBrowser<@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:332:9

It may only happen when shaking the RDM on and off.

	Alexandre Poirot [:ochameau]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8912458 [details]
Bug 1305777 - Move RDM frame script to new RDM.

https://reviewboard.mozilla.org/r/183782/#review189206

Thanks for the multiple patches, it really helped review such work!

	Alexandre Poirot [:ochameau]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8912459 [details]
Bug 1305777 - Remove RDM manager switch and pref.

https://reviewboard.mozilla.org/r/183784/#review189208

	Alexandre Poirot [:ochameau]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8912460 [details]
Bug 1305777 - Move RDM GCLI commands to new RDM.

https://reviewboard.mozilla.org/r/183786/#review189210

	Alexandre Poirot [:ochameau]
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8912461 [details]
Bug 1305777 - Remove old RDM.

https://reviewboard.mozilla.org/r/183788/#review189212

	Alexandre Poirot [:ochameau]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8912462 [details]
Bug 1305777 - Clean up tests that supported both RDMs.

https://reviewboard.mozilla.org/r/183790/#review189214

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

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Thanks, it seems to look good. I tried to manual test everything and haven't
> seen any glitch.

Thanks for all the reviews!

> Should we also remove these files as well?
>   devtools/client/themes/responsivedesign.css
>   devtools/client/locales/en-US/responsiveUI.properties

Yes, thanks for catching that.  I also found:

* bits of CSS in browser.css
* images for the old UI

Guess I went a little too fast! :/ I'll remove them too.

> And also: devtools/shared/touch/simulator.js and
> devtools/shared/touch/simulator-content.js
> It looks like only devtools/shared/touch/simulator-core.js is used by
> EmulationActor (which is the way to do touch for responsive.html).

Another good catch!  I'll post an extra patch for this part.

> I got this exception, but can't easily reproduce:
> Full message: TypeError: this.emulationFront is undefined
> Full stack:
> ResponsiveUI.prototype.updateTouchSimulation<@resource://devtools/shared/
> base-loader.js -> resource://devtools/client/responsive.html/manager.js:558:7
> _run@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/task.js:310:39
> TaskImpl@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/task.js:272:3
> asyncFunction@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/task.js:246:14
> onChangeTouchSimulation@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/responsive.html/manager.js:499:5
> handleMessage@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/responsive.html/manager.js:462:9
> handleEvent@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/responsive.html/manager.js:434:9
> EventListener.handleEvent*ResponsiveUI.prototype.init</this.swap<.
> getInnerBrowser<@resource://devtools/shared/base-loader.js ->
> resource://devtools/client/responsive.html/manager.js:332:9
> 
> It may only happen when shaking the RDM on and off.

Hmm, it doesn't seem related to the removing old RDM at least...  Maybe if you toggle while also toggling RDM itself quickly?  If we find a good STR, let's file a separate bug.

	Alexandre Poirot [:ochameau]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8912940 [details]
Bug 1305777 - Clean up touch simulator after old RDM removal.

https://reviewboard.mozilla.org/r/184264/#review189432

devtools/server/touch-simulator.js isn't declared as a copy of simulator-core.js?
Are you using cinnabar? If yes, you need glandium's magick trick...

Otherwise, /devtools/server is quite messy. We tend to just drop things either in server/ or actors/utils/ folders.
What about putting touch-simulator in devtools/server/actors/emulation/touch-simulator? It helps following that's just a emulation actor dependency.

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

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> devtools/server/touch-simulator.js isn't declared as a copy of
> simulator-core.js?
> Are you using cinnabar? If yes, you need glandium's magick trick...

Thanks for the tip, seems like it would be better as a Git config option to avoid editing cinnabar...

> Otherwise, /devtools/server is quite messy. We tend to just drop things
> either in server/ or actors/utils/ folders.
> What about putting touch-simulator in
> devtools/server/actors/emulation/touch-simulator? It helps following that's
> just a emulation actor dependency.

I agree, moved!

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31bd73af6db36ef2f336cd2cdfb226b9ff1f635c

	Julian Descottes [:jdescottes]
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8913292 [details]
Bug 1305777 - Fix ESLint issues for moved files.

https://reviewboard.mozilla.org/r/184662/#review189838

	Pulsebot
	Comment 28 • 7 years ago

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5a0227d048e8
Move RDM frame script to new RDM. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6ae05de7437d
Remove RDM manager switch and pref. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/776ad30a20db
Move RDM GCLI commands to new RDM. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6e0cb1927bfe
Remove old RDM. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c311ac4b05b7
Clean up tests that supported both RDMs. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/0169ee5c78fb
Clean up touch simulator after old RDM removal. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ffb4e22a4347
Fix ESLint issues for moved files. r=jdescottes

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 29 • 7 years ago

Backed out for failing devtools' devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js on Windows 7 debug without e10s:

https://hg.mozilla.org/integration/autoland/rev/1fe25bc278e75281a56b0e400f55a0f73bc5635e
https://hg.mozilla.org/integration/autoland/rev/883484b7794765b796e9eeeef38a958b7a6052ae
https://hg.mozilla.org/integration/autoland/rev/4e494ae5b19d9280c2c228f5340fd5f2d405cc0a
https://hg.mozilla.org/integration/autoland/rev/27bf9dc1839e5feb9bb59c1c554819319e39aa49
https://hg.mozilla.org/integration/autoland/rev/d9d93f156d3b3afe7a70e6b867e503c653586107
https://hg.mozilla.org/integration/autoland/rev/3ff120562b4a06a3eb829901771e02d1f8168fd2
https://hg.mozilla.org/integration/autoland/rev/84606fe1cd2df32342f4d3eb512bf911e2151cd0

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ffb4e22a4347adfd47536ccec04d3b2e95c8f2c7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133983113&repo=autoland

04:15:49     INFO -  697 INFO Testing if media rules have the appropriate number of links
04:15:49     INFO -  698 INFO TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js | There should be no links in the first media rule. -
04:15:49     INFO -  699 INFO TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js | There should be no links in the second media rule. -
04:15:49     INFO -  700 INFO TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js | There should be 1 responsive mode link in the media rule -
04:15:49     INFO -  701 INFO TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js | There should be 2 responsive mode links in the media rule -
04:15:49     INFO -  702 INFO Waiting for event: 'media-list-changed' on [object Object].
04:15:49     INFO -  703 INFO Launching responsive mode
04:15:49     INFO -  704 INFO Waiting for content-resize to a width of 400
04:15:49     INFO -  Buffered messages finished
04:15:49    ERROR -  705 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js:65 - TypeError: rdmUI is undefined
04:15:49     INFO -  Stack trace:
04:15:49     INFO -      testMediaLink@chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js:65:3
04:15:49     INFO -      process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
04:15:49     INFO -      walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
04:15:49     INFO -      Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
04:15:49     INFO -      schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
04:15:49     INFO -      completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:315:13
04:15:49     INFO -      promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
04:15:49     INFO -      process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
04:15:49     INFO -      walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
04:15:49     INFO -      Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
04:15:49     INFO -      schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
04:15:49     INFO -      completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
04:15:49     INFO -      onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1356:9
04:15:49     INFO -      DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1296:14
04:15:49     INFO -      generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1454:14
04:15:49     INFO -      PerformanceFront<.connect<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/fronts/performance.js:34:28
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
04:15:49     INFO -      process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
04:15:49     INFO -      walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
04:15:49     INFO -      Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
04:15:49     INFO -      schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
04:15:49     INFO -      completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:315:13
04:15:49     INFO -      promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
04:15:49     INFO -      promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
04:15:49     INFO -      TaskImpl@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:272:3
04:15:49     INFO -      asyncFunction@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:246:14
04:15:49     INFO -      StyleEditorUI.prototype.initialize<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:144:11
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
04:15:49     INFO -      process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
04:15:49     INFO -      walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
04:15:49     INFO -      Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
04:15:49     INFO -      schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
04:15:49     INFO -      completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
04:15:49     INFO -      onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1356:9
04:15:49     INFO -      DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1296:14
04:15:49     INFO -      generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1454:14
04:15:49     INFO -      StyleEditorUI.prototype.initialize<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:143:29
04:15:49     INFO -      _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
04:15:49     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
04:15:49     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
04:15:49     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
04:15:49     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
04:15:49     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
04:15:49     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e8bdcf976ecf5612f83da6c1ac74db2e1253058

	Pulsebot
	Comment 39 • 7 years ago

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc55e32ba087
Move RDM frame script to new RDM. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/71faac859b4a
Remove RDM manager switch and pref. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/3c1ab86a8611
Move RDM GCLI commands to new RDM. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d939f110bf2e
Remove old RDM. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a2c2643cf909
Clean up tests that supported both RDMs. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e2ea4cf82cae
Clean up touch simulator after old RDM removal. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/3b5b98033433
Fix ESLint issues for moved files. r=jdescottes

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 40 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fc55e32ba087
https://hg.mozilla.org/mozilla-central/rev/71faac859b4a
https://hg.mozilla.org/mozilla-central/rev/3c1ab86a8611
https://hg.mozilla.org/mozilla-central/rev/d939f110bf2e
https://hg.mozilla.org/mozilla-central/rev/a2c2643cf909
https://hg.mozilla.org/mozilla-central/rev/e2ea4cf82cae
https://hg.mozilla.org/mozilla-central/rev/3b5b98033433

	Oriol Brufau [:Oriol]
	Comment 41 • 7 years ago

If you remove the old RDM you could at least make the new one support non-e10s.

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

(In reply to Oriol Brufau [:Oriol] from comment #41)
> If you remove the old RDM you could at least make the new one support
> non-e10s.

I am not aware of strong need for this.  e10s is now enabled everywhere by default (minus possibly a11y for now? but it's on the way).  This is exactly why we waited until now before removing the old RDM.

If you have a use case in mind where non-e10s is needed with new RDM, please file a new bug and we can discuss there.

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

Marking "dev-doc-needed" to consider removing the docs about the pre-52 version:

https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode_(before_Firefox_52)

and remove the link to that page from the new docs:

https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode

	Chris Mills [:cmills]
	Comment 44 • 7 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #43)
> Marking "dev-doc-needed" to consider removing the docs about the pre-52
> version:
> 
> https://developer.mozilla.org/en-US/docs/Tools/
> Responsive_Design_Mode_(before_Firefox_52)
> 
> and remove the link to that page from the new docs:
> 
> https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode

Done. 

I've also added a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#Developer_Tools