Closed Bug 1305777 Opened 7 years ago Closed 6 years ago

Remove old RDM

Categories

(DevTools :: Responsive Design Mode, defect, P3)

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

Bug 1305777 - Move RDM frame script to new RDM.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1305777 - Remove RDM manager switch and pref.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1305777 - Move RDM GCLI commands to new RDM.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1305777 - Remove old RDM.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1305777 - Clean up tests that supported both RDMs.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1305777 - Clean up touch simulator after old RDM removal.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1305777 - Fix ESLint issues for moved files.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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
QA Whiteboard: [qe-rdm]
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.
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.
Now that e10s has become the default, it's likely a good time to do this.
: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
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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.
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!
Attachment #8912458 - Flags: review?(poirot.alex) → review+
Comment on attachment 8912459 [details]
Bug 1305777 - Remove RDM manager switch and pref.

https://reviewboard.mozilla.org/r/183784/#review189208
Attachment #8912459 - Flags: review?(poirot.alex) → review+
Comment on attachment 8912460 [details]
Bug 1305777 - Move RDM GCLI commands to new RDM.

https://reviewboard.mozilla.org/r/183786/#review189210
Attachment #8912460 - Flags: review?(poirot.alex) → review+
Comment on attachment 8912461 [details]
Bug 1305777 - Remove old RDM.

https://reviewboard.mozilla.org/r/183788/#review189212
Attachment #8912461 - Flags: review?(poirot.alex) → review+
Comment on attachment 8912462 [details]
Bug 1305777 - Clean up tests that supported both RDMs.

https://reviewboard.mozilla.org/r/183790/#review189214
Attachment #8912462 - Flags: review?(poirot.alex) → review+
(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.
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.
Attachment #8912940 - Flags: review?(poirot.alex) → review+
(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!
Comment on attachment 8913292 [details]
Bug 1305777 - Fix ESLint issues for moved files.

https://reviewboard.mozilla.org/r/184662/#review189838
Attachment #8913292 - Flags: review?(jdescottes) → review+
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
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
Flags: needinfo?(jryans)
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
If you remove the old RDM you could at least make the new one support non-e10s.
Depends on: 1404197
(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.
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
Keywords: dev-doc-needed
(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
Keywords: dev-doc-needed → dev-doc-complete
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.