Closed
Bug 1446975
Opened 6 years ago
Closed 6 years ago
Synced Tabs sidebar device icons should match those in page action menu
Categories
(Firefox :: Sync, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: rfeeley, Assigned: amychan331, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(2 files)
Now that we have all the Photon icons required for Sync devices in the page action menu, let's reuse them in the Synced Tabs sidebar. Attached is current state.
Comment 1•6 years ago
|
||
Steps to work on that bug: 1. Replace the use of the old icons [0] by the new ones [1] 2. Remove the files and references of the old icons [2] 3. (brownie-points/optional): use the tablet icon (device-tablet.svg) for tablets, the investigation starting points are here [3]. It is way more involved so I don't expect the taker of this bug to do it. I'll probably open a follow-up bug once this one is resolved. [0] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/themes/shared/syncedtabs/sidebar.inc.css#92,102 [1] chrome://browser/skin/device-desktop.svg and chrome://browser/skin/device-mobile.svg [2] https://searchfox.org/mozilla-central/search?q=sync-desktopIcon.svg and https://searchfox.org/mozilla-central/search?q=sync-mobileIcon.svg [3] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/services/sync/modules/SyncedTabs.jsm#79 https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/base/content/browser-sync.js#389-390
Mentor: eoger
Keywords: good-first-bug
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•6 years ago
|
||
Hi, can I do this one? I would be happy to try out the tablet icon in [3] too!
Comment 3•6 years ago
|
||
(In reply to Amy from comment #2) > Hi, can I do this one? I would be happy to try out the tablet icon in [3] > too! Yes, please do! Feel free to reach out if you need additional help, but otherwise follow the steps in comment 1, then put a patch up for review and we'll assign the bug to you.
Updated•6 years ago
|
Assignee: nobody → tchiovoloni
Assignee | ||
Comment 4•6 years ago
|
||
I got really busy with some events this weekend and haven't been able to work on it - the task is simple, but I am having some problem with mach. Does the reassignment means I shouldn't work on this?
Comment 5•6 years ago
|
||
Hi Amy, If you think you will be able to make progress over the next 2 weeks, we'll happily leave it for you to do!
Assignee: tchiovoloni → nobody
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
I should be able to - it seems that rebuilding the mach fixes the problem, although it took a while on my very old macbook. I completed step 1 and 2, and I just hg pushed it to mozreview (reviewer's the bug mentor, right? Not the reporter or triage owner?). If that works fine, I will try to tackle step 3.
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review237582 I've tried it on my computer and it looks awesome, thank you! ::: commit-message-3d21d:1 (Diff revision 1) > +Bug 1446975 - Replace synced Tabs sidebar device icons; r?[:eoger] Nitpick: . r?eoger instead ::: commit-message-3d21d:1 (Diff revision 1) > +Bug 1446975 - Replace synced Tabs sidebar device icons; r?[:eoger] Nitpick: . r?eoger instead
Attachment #8963017 -
Flags: review?(eoger) → review+
Comment 9•6 years ago
|
||
> I should be able to - it seems that rebuilding the mach fixes the problem, although it took a while on my very old macbook. You could try artifact builds [0], changes like this don't need a full build. > If that works fine, I will try to tackle step 3. Great! I'll leave the bug open then. [0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds
Updated•6 years ago
|
Assignee: nobody → amy_yyc
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
Ok! I finally got the tablet icon in. For a while, I was wondering why none of my changes seemed to have any effect, but then I realize I didn't do any mach re-build... After that, the changes appeared. First time trying to do a mozilla problem that involve digging into multiple files instead of simple one-file fix. Hope I did ok!
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review238426 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/modules/engines/clients.js:240 (Diff revision 2) > return null; > }, > > + isTablet: function isTablet(id) { > + if (this._store._remoteClients[id]) { > + return this._store._remoteClients[id].formfactor.includes(DEVICE_TYPE_TABLET); Error: 'device_type_tablet' is not defined. [eslint: no-undef]
Updated•6 years ago
|
Attachment #8963017 -
Flags: review+ → review?(eoger)
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review238966 This is good first-cut and it looks like you have idenfied the paths we take to show these icons, good job! :) I have left comments, mostly to factorize the code and remove dead code. ::: browser/themes/shared/syncedtabs/sidebar.inc.css:111 (Diff revisions 1 - 2) > > .item.client.device-image-mobile.selected:focus > .item-title-container > .item-icon-container { > fill: white; > } > > +.item.client.device-image-tablet > .item-title-container > .item-icon-container { Can we factorize these two rules with the ones on top? ::: services/sync/modules/SyncedTabs.jsm:79 (Diff revision 2) > async _makeClient(client) { > return { > id: client.id, > type: "client", > name: Weave.Service.clientsEngine.getClientName(client.id), > + isTablet: Weave.Service.clientsEngine.isTablet(client.id), How about having a |formFactor| key instead, that can take 3 values: "desktop" (default), "tablet", and "mobile". ::: services/sync/modules/engines/clients.js:209 (Diff revision 2) > this.fxAccounts.updateDeviceRegistration().catch(error => { > this._log.warn("failed to update fxa device registration", error); > }); > }, > > + get localFormfactor() { Do we use these anywhere? ::: services/sync/modules/engines/clients.js:238 (Diff revision 2) > return this._store._remoteClients[id].fxaDeviceId; > } > return null; > }, > > + isTablet: function isTablet(id) { I think we should try to avoid duplication and come up with a "getFormFactor" method. The logic would be something like this: https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/browser/base/content/browser-sync.js#387-388 Note: there is another user of isMobile! https://searchfox.org/mozilla-central/search?q=clientsEngine.isMobile ::: services/sync/modules/engines/clients.js:240 (Diff revision 2) > return null; > }, > > + isTablet: function isTablet(id) { > + if (this._store._remoteClients[id]) { > + return this._store._remoteClients[id].formfactor.includes(DEVICE_TYPE_TABLET); You have to add DEVICE_TYPE_TABLET to https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/tools/lint/eslint/modules.json#32 (sorry) ::: services/sync/modules/engines/clients.js:976 (Diff revision 2) > } catch (error) { > this._log.warn("failed to get fxa device id", error); > } > record.name = this.engine.localName; > record.type = this.engine.localType; > + record.formfactor = this.engine.formfactor; I don't think this is needed. (we should probably write that property though, let's do that in another bug). ::: services/sync/modules/util.js:36 (Diff revision 2) > > XPCOMUtils.defineLazyPreferenceGetter(this, "localDeviceType", > "services.sync.client.type", > DEVICE_TYPE_DESKTOP); > > +XPCOMUtils.defineLazyPreferenceGetter(this, "localDeviceFormFactor", This can be removed. ::: services/sync/modules/util.js:717 (Diff revision 2) > > getDeviceType() { > return localDeviceType; > }, > > + getDeviceFormFactor() { Ditto
Attachment #8963017 -
Flags: review?(eoger)
Comment 14•6 years ago
|
||
Oh, and also if you could amend the tests in |browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js| that would be great! I think you only need to change the |checkItem| method to check for the class the client node gets assigned.
Comment 15•6 years ago
|
||
(In reply to Edouard Oger [:eoger] from comment #13) > ::: services/sync/modules/SyncedTabs.jsm:79 > (Diff revision 2) > > async _makeClient(client) { > > return { > > id: client.id, > > type: "client", > > name: Weave.Service.clientsEngine.getClientName(client.id), > > + isTablet: Weave.Service.clientsEngine.isTablet(client.id), > > How about having a |formFactor| key instead, that can take 3 values: > "desktop" (default), "tablet", and "mobile". I agree that we should move to a new |formFactor| concept. However, I'm not sure "mobile" makes sense in that context - how about simply "desktop" (which is for device type="desktop") plus "tablet" and "phone" (used for device type="mobile")? > You have to add DEVICE_TYPE_TABLET to > https://searchfox.org/mozilla-central/rev/ > a0665934fa05158a5a943d4c8b277465910c029c/tools/lint/eslint/modules.json#32 > (sorry) If we go that route, we possibly want DEVICE_FORMFACTOR_* constants rather than confusingly conflating "type" and "formfactor"?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review238966 > Can we factorize these two rules with the ones on top? I used [class*="device-image-"] to combine the css rules that are shared. I can't use it for the background-image rule though.
Assignee | ||
Comment 18•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review238966 > I think we should try to avoid duplication and come up with a "getFormFactor" method. > The logic would be something like this: https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/browser/base/content/browser-sync.js#387-388 > > Note: there is another user of isMobile! > https://searchfox.org/mozilla-central/search?q=clientsEngine.isMobile Apparently formfactor can be phone, smalltablet, largetablet, laptop, or tv, so the if-else statement checks if it's phone or contains tablet before defaulting to desktop. Since isMobile is use elsewhere, I didn't touch it. Should I leave isMobile alone, or should I remove it?
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #15) > I agree that we should move to a new |formFactor| concept. However, I'm not > sure "mobile" makes sense in that context - how about simply "desktop" > (which is for device type="desktop") plus "tablet" and "phone" (used for > device type="mobile")? What do you mean by "desktop" plus "tablet" and "phone"?
Comment hidden (mozreview-request) |
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review239950 Please let me know if any of these comments don't make sense. ::: browser/components/syncedtabs/TabListView.js:227 (Diff revision 3) > if (item.selected) { > itemNode.classList.add("selected"); > } else { > itemNode.classList.remove("selected"); > } > - if (item.isMobile) { > + if (item.formFactor) { With the changes to SyncedTabs I suggest below, we probably don't need the "else" part of this - the values returned by SyncedTabs.jsm will always have one of 3 string values for formFactor. ::: services/sync/modules/SyncedTabs.jsm:79 (Diff revision 3) > async _makeClient(client) { > return { > id: client.id, > type: "client", > name: Weave.Service.clientsEngine.getClientName(client.id), > - isMobile: Weave.Service.clientsEngine.isMobile(client.id), > + formFactor: Weave.Service.clientsEngine.getFormFactor(client.id), IMO it is here that the logic for translating the Sync concepts into the "ui concepts" should live. IOW, the Sync modules always return the "raw" values. This module translates them into concepts which make it easy for the UI. So something like: ``` let formFactor; if (Weave.Service.clientsEngine.isMobile(client.id)) { // mobile devices have a formfactor. if (Weave.Service.clientsEngine.getFormFactor(client.id) == DEVICE_FORMFACTOR_PHONE) { formFactor = "phone"; } else { // for now, assume anything else is a tablet. formFactor = "tablet"; } } else { formFactor = "desktop"; } return { ... formFactor, ... }; ``` (and as I mention below, it might even be OK to use a literal instead of DEVICE_FORMFACTOR_PHONE, but Ed will have his own thoughts) ::: services/sync/modules/bookmark_repair.js:489 (Diff revision 3) > > /* Is the passed client record suitable as a repair responder? > */ > _isSuitableClient(client) { > // filter only desktop firefox running > 53 (ie, any 54) > - return (client.type == DEVICE_TYPE_DESKTOP && > + return (client.type == DEVICE_FORMFACTOR_DESKTOP && We don't need this change. ::: services/sync/modules/constants.js:130 (Diff revision 3) > kSyncBackoffNotMet: "Trying to sync before the server said it's okay", > kFirstSyncChoiceNotMade: "User has not selected an action for first sync", > kSyncNotConfigured: "Sync is not configured", > kFirefoxShuttingDown: "Firefox is about to shut down", > > -DEVICE_TYPE_DESKTOP: "desktop", > +DEVICE_FORMFACTOR_DESKTOP: "desktop", I don't think we want these here, as they don't correspond with the formFactor values actually written by mobile clients. TBH, it might be fine just using literal strings everywhere, but please take Ed's advice when he has a look. ::: services/sync/modules/engines/clients.js:146 (Diff revision 3) > }, > > // Aggregate some stats on the composition of clients on this account > get stats() { > let stats = { > - hasMobile: this.localType == DEVICE_TYPE_MOBILE, > + hasMobile: this.localType == DEVICE_FORMFACTOR_MOBILE, This change and the next 2 below should be removed. ::: services/sync/modules/engines/clients.js:231 (Diff revision 3) > return this._store._remoteClients[id].fxaDeviceId; > } > return null; > }, > > + getFormFactor: function getFormFactor(id) { Please change this to use the more modern signature of a plain `getFormFactor(id) {...`, similar to the other functions. But this should just return the formFactor field from the record directly, or null if it is undefined. That will mean that devices where isMobile() returns false will have null for a formFactor, but that's OK - it's SyncedTabs.jsm where the abstraction into the more useful UI values should live. ::: services/sync/modules/engines/clients.js:243 (Diff revision 3) > + return false; > + }, > + > isMobile: function isMobile(id) { > if (this._store._remoteClients[id]) > - return this._store._remoteClients[id].type == DEVICE_TYPE_MOBILE; > + return this._store._remoteClients[id].type == DEVICE_FORMFACTOR_MOBILE; This change and the rest of the changes in this file should be dropped. ::: services/sync/modules/util.js:34 (Diff revision 3) > "services.sync.client.name", > ""); > > XPCOMUtils.defineLazyPreferenceGetter(this, "localDeviceType", > "services.sync.client.type", > - DEVICE_TYPE_DESKTOP); > + DEVICE_FORMFACTOR_DESKTOP); This should be dropped.
Comment 22•6 years ago
|
||
(In reply to Amy from comment #19) > (In reply to Mark Hammond [:markh] from comment #15) > > > I agree that we should move to a new |formFactor| concept. However, I'm not > > sure "mobile" makes sense in that context - how about simply "desktop" > > (which is for device type="desktop") plus "tablet" and "phone" (used for > > device type="mobile")? > > What do you mean by "desktop" plus "tablet" and "phone"? Sorry, I was too hasty in my reply. I meant that in general, a desktop device should be considered to have a formfactor of "desktop" - which your patch partially does. However, I think your patch goes too far - we want to keep *both* "device type" and "form factor" concepts. If a deviceType=="mobile", then the formFactor will be one of the values you mention (phone, smalltablet, largetablet, laptop, or tv) - but they all come directly from the mobile record. If the deviceType=="desktop", then those records do *not* have a formFactor field - but it should be logically treated as though there was a formFactor field called "desktop". I'll make a few more notes in the patch.
Comment 23•6 years ago
|
||
(sorry, but bugzilla tricked me :) It will probably make more sense to read comment 22 before comment 21
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•6 years ago
|
||
Ok, I think I got all the new changes in! Please let me know if I missed anything and if the current changes are good!
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review240412 This is on the right track and looks good. ::: browser/components/syncedtabs/TabListView.js:230 (Diff revision 5) > - itemNode.classList.add("device-image-desktop"); > - } > if (item.focused) { > itemNode.focus(); > } > + itemNode.classList.add("device-image-" + item.formFactor); I'm a bit un-easy with the concatenation here since formFactor values might change in the future. It doesn't bother me too much however, let's try to use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals at least. ::: browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js:9 (Diff revision 5) > { > "id": "7cqCr77ptzX3", > "type": "client", > "lastModified": 1492201200, > "name": "zcarter's Nightly on MacBook-Pro-25", > - "isMobile": false, > + "formFactor": false, We want a different formfactor here and after. False is not a legal value. Also we should change |checkItem| to verify the applied class. ::: browser/themes/shared/syncedtabs/sidebar.inc.css:91 (Diff revision 5) > > .item.tab > .item-title-container { > padding-inline-start: 20px; > } > > -.item.client.device-image-desktop > .item-title-container > .item-icon-container { > +.item.client[class*="device-image-"] > .item-title-container > .item-icon-container { |class*| is probably slower than a boring .class selector, can we explicitly list the classes instead? ::: services/sync/modules/util.js (Diff revision 5) > ""); > > XPCOMUtils.defineLazyPreferenceGetter(this, "localDeviceType", > "services.sync.client.type", > DEVICE_TYPE_DESKTOP); > - Leave that space
Attachment #8963017 -
Flags: review?(eoger)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review240412 > We want a different formfactor here and after. False is not a legal value. > Also we should change |checkItem| to verify the applied class. Is undefined an acceptable value in place of false? For |checkItem|, I added an assert check for device-image-${item.formFactor} if the input item is a client.
Comment 29•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review240648 ::: browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js:401 (Diff revision 6) > "Node's title element's text should match client name"); > Assert.ok(node.classList.contains("client"), > "Node should have .client class"); > Assert.equal(node.dataset.id, item.id, > "Node's ID should match item ID"); > + if (item.formFactor) { This will result in testing for the class "device-image-undefined" with the current fixture. Let's just remove that test then, and give legal formFactors (mobile, desktop, tablet) to the fixture. ::: browser/themes/shared/syncedtabs/sidebar.inc.css:93 (Diff revision 6) > padding-inline-start: 20px; > } > > -.item.client.device-image-desktop > .item-title-container > .item-icon-container { > - background-image: url("chrome://browser/skin/sync-desktopIcon.svg"); > +.item.client.device-image-desktop, > +.item.client.device-image-tablet, > +.item.client.device-image-mobile > .item-title-container > .item-icon-container You want the full selector in each case ::: browser/themes/shared/syncedtabs/sidebar.inc.css:100 (Diff revision 6) > -moz-context-properties: fill; > fill: #4d4d4d; > } > > -.item.client.device-image-desktop.selected:focus > .item-title-container > .item-icon-container { > +.item.client.device-image-desktop.selected:focus, > +.item.client.device-image-tablet.selected:focus, Same
Attachment #8963017 -
Flags: review?(eoger)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review240648 > This will result in testing for the class "device-image-undefined" with the current fixture. > Let's just remove that test then, and give legal formFactors (mobile, desktop, tablet) to the fixture. Reverted, and fixture formFactors are given an a legal value, though the item name implies that they are all just "desktop".
Comment 32•6 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/44e3e7446b02 Replace synced Tabs sidebar device icons. r=eoger
Comment 33•6 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/69653800c483 fix eslint failure
Comment 34•6 years ago
|
||
Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff301ae67fe8 Backed out 2 changesets for failing xpcshell on sync/tests/unit/test_syncedtabs.js and browser-chrome failures e.g. test/urlbar/browser_page_action_menu.js on a CLOSED TREE
Comment 35•6 years ago
|
||
Backed out 2 changesets (bug 1446975) for failing xpcshell on sync/tests/unit/test_syncedtabs.js and browser-chrome failures e.g. test/urlbar/browser_page_action_menu.js on a CLOSED TREE Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff301ae67fe8d9dc43afa1aab187538881da1262 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=44e3e7446b02decfd30b66795544441b11f1ae88 Log link for xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=173353491&repo=mozilla-inbound&lineNumber=2594 Log link for browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=173353047&repo=mozilla-inbound&lineNumber=3433 Log snippet for xpcshell: [task 2018-04-12T21:22:45.442Z] 21:22:45 INFO - TEST-START | services/sync/tests/unit/test_clients_engine.js [task 2018-04-12T21:22:54.774Z] 21:22:54 INFO - TEST-PASS | services/sync/tests/unit/test_clients_engine.js | took 9333ms [task 2018-04-12T21:22:54.782Z] 21:22:54 INFO - Retrying tests that failed when run in parallel. [task 2018-04-12T21:22:54.790Z] 21:22:54 INFO - TEST-START | services/sync/tests/unit/test_syncedtabs.js [task 2018-04-12T21:22:55.581Z] 21:22:55 WARNING - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_syncedtabs.js | xpcshell return code: 0 Log snippet for browser-chrome: [task 2018-04-12T21:15:24.761Z] 21:15:24 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "Learn About Sending Tabs..." == "Learn About Sending Tabs..." - [task 2018-04-12T21:15:24.762Z] 21:15:24 INFO - Leaving test bound sendToDevice_noDevices [task 2018-04-12T21:15:24.764Z] 21:15:24 INFO - Entering test bound sendToDevice_devices [task 2018-04-12T21:15:24.765Z] 21:15:24 INFO - Buffered messages logged at 21:15:24 [task 2018-04-12T21:15:24.767Z] 21:15:24 INFO - Waiting for main page action button to have non-0 size [task 2018-04-12T21:15:24.769Z] 21:15:24 INFO - Waiting for main page action panel to be open [task 2018-04-12T21:15:24.770Z] 21:15:24 INFO - promiseNodeVisible waiting, node.id=pageAction-panel-bookmark node.localeName=toolbarbutton [task 2018-04-12T21:15:24.771Z] 21:15:24 INFO - [task 2018-04-12T21:15:24.772Z] 21:15:24 INFO - promiseNodeVisible OK, node.id=pageAction-panel-bookmark node.localeName=toolbarbutton [task 2018-04-12T21:15:24.773Z] 21:15:24 INFO - [task 2018-04-12T21:15:24.776Z] 21:15:24 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - [task 2018-04-12T21:15:24.776Z] 21:15:24 INFO - promisePageActionViewShown waiting for ViewShown [task 2018-04-12T21:15:24.777Z] 21:15:24 INFO - Buffered messages finished [task 2018-04-12T21:15:24.779Z] 21:15:24 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_page_action_menu.js | uncaught exception - TypeError: client is undefined at getClientType@resource://services-sync/engines/clients.js:237:1
Flags: needinfo?(amy_yyc)
Comment 37•6 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdc093a1727 Replace synced Tabs sidebar device icons. r=eoger
Comment 38•6 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3c36ffacd8 Replace synced Tabs sidebar device icons. r=eoger
Comment 39•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dfdc093a1727 https://hg.mozilla.org/mozilla-central/rev/5c3c36ffacd8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 40•6 years ago
|
||
mozreview-review |
Comment on attachment 8963017 [details] Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger https://reviewboard.mozilla.org/r/231858/#review242210
Attachment #8963017 -
Flags: review?(eoger) → review+
Comment 41•6 years ago
|
||
Thank you Amy, your patch is now merged!
Assignee | ||
Comment 42•6 years ago
|
||
Glad that I got to worked with it! I have updated my MozillaReview requests to all fixed. Is there anything else I should do? I have only work with the review board 2 times, and the last one closed the request automatically.
Comment 43•6 years ago
|
||
Nothing to do! I landed the patch for you, it should be in Nightly :)
You need to log in
before you can comment on or make changes to this bug.
Description
•