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)

Product:
Firefox
Component:
Sync
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: rfeeley, Assigned: amychan331, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

sidebar.png
86.44 KB, image/png
Details
Bug 1446975 - Replace synced Tabs sidebar device icons; :eoger
59 bytes, text/x-review-board-request
eoger
: review+
Details
Attached image sidebar.png — 
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.
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
Priority: -- → P2
Hi, can I do this one? I would be happy to try out the tablet icon in [3] too!
(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.
Assignee: nobody → tchiovoloni
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?
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
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 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+
> 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
Assignee: nobody → amy_yyc
Status: NEW → ASSIGNED
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 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]
Attachment #8963017 - Flags: review+ → review?(eoger)
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)
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.
(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 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.
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?
(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 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.
(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.
(sorry, but bugzilla tricked me :) It will probably make more sense to read comment 22 before comment 21
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 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 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 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 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".
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e3e7446b02
Replace synced Tabs sidebar device icons. r=eoger
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
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)
Will re-push.
Flags: needinfo?(amy_yyc)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdc093a1727
Replace synced Tabs sidebar device icons. r=eoger
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3c36ffacd8
Replace synced Tabs sidebar device icons. r=eoger
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 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+
Thank you Amy, your patch is now merged!
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: