Closed Bug 1585290 Opened 1 year ago Closed 8 months ago

theme.reset clears window theme even if another extension had called theme.update

Categories

(WebExtensions :: Themes, defect, P5)

defect

Tracking

(firefox76 fixed)

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: robwu, Assigned: myeongjun.ko, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

The implementation of theme.reset has a check at https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/toolkit/components/extensions/parent/ext-theme.js#478

Since defaultTheme is never falsey, there is never an early return, and any call to theme.reset unconditionally clears the theme of the window.

The condition should probably be changed to: "if override is set and the override is set by the current extension". This matches the documented behavior: "Resets any theme that was applied using the theme.update() method." ( https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/theme/reset ), and also the behavior of of theme.reset before a patch broke it (https://searchfox.org/mozilla-central/diff/fef7bcd6aacbdf241adeb9502f966e90abe6b073/toolkit/components/extensions/ext-theme.js#383):

The condition should be changed to:

  let theme = windowOverrides.get( ... );
  if (!theme || theme.extension !== extension) {
    // theme.update hasn't been invoked.
    return;
  }

or, if calling theme.reset is always supposed to reset the window to the default theme:

  let theme = windowOverrides.get( ... );
  if (theme && theme.extension !== extension) {
    // Theme was set by another extension, leave it be.
    return;
  }

... and a unit test should be added to ensure that the expected behavior happens.

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Priority: -- → P3

Marking as good-first-bug.

The first comment already shows how this bug can be fixed.

Those who are interested in working on this good-first-bug can get started with the tutorial at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Mentor: rob
Keywords: good-first-bug
Priority: P3 → P5

Hello.
I have a question about your guidelines.

There are two ways to modify it.

  1. theme.update hasn't been invoked.
  2. Theme was set by another extension, leave it be.

If I modify code as follows, Is it satisfied with the above two cases?

let theme = windowOverrides.get( ... );
if (!theme || theme.extension !== extension) {
     return;
}

If I solve the issue, should I fill out the test case on there[0]?
I'll get confirmation from you and I gonna try to solve this issue.

[0]
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js

Flags: needinfo?(rob)
Flags: needinfo?(rob)

I submitted patch. I think it be insufficient.
If you have a time, Let me know if there's anything wrong with a misprint or something after reviewing code.

Flags: needinfo?(rob)

(In reply to Myeongjun Go from comment #6)

I submitted patch.

Thanks! I've assigned the bug and reviewed the patch.

I think it be insufficient.

I think that you meant to say "sufficient". "insufficient" means that you believe that the patch has is incomplete.

Assignee: nobody → myeongjun.ko
Status: NEW → ASSIGNED
Flags: needinfo?(rob)

Hello Rob.
I used checkin-needed keyword to apply patch. But I can't find the keyword.
Instead, I found other keywords(i.e.checkin-needed-tb) and information about the keyword.
The keyword info said that Do not use for Phabricator patches, or Firefox patches.
What proper keywords can I use to apply patches?

Flags: needinfo?(rob)

The checkin-needed keyword was recently removed.

Do you see a "View Stack in Lando" option in Phabricator?
And if so, are you able to land the patch from there?

Flags: needinfo?(rob)

Let me know if you are able to add that tag. If not, then I can land the patch for you.
I'm just curious whether new contributors can add the tag - if you can, then I will update the information on the wiki.

Flags: needinfo?(myeongjun.ko)

I entered into "View Stack in Lando" menu and clicked Privew Landing for landing.
And then I saw that
Landing is Blocked: You have insufficient permissions to land. Level 3 Commit Access is required. See the FAQ for help.

I looked for another way. But It looks the same way I've just tried [0].
So, I can't land the patch now.
Let me know if you have other workflow. I'll refer to it.

[0]
https://moz-conduit.readthedocs.io/en/latest/lando-user.html

Flags: needinfo?(myeongjun.ko)

Lando doesn't work for you, but are you able to add the "Check-in Needed" tag to Phabricator after clicking on Edit revision?

Edit revision = https://wiki.mozilla.org/File:Check-in_needed_phabricator_edit_revision.png

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92914b96908c
theme.reset clears window theme even if another extension had called r=robwu

I added Check-in Needed tag.
If there's anything you want to check on, feel free to ask me.
Thank you.

Flags: needinfo?(rob)

Adding Check-in Needed seems to have worked.
Unfortunately, the patch has been reverted ("backed out") because of test failures.

Could you look into the failing tests (run them locally) and update the patch?

Flags: needinfo?(rob)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:myeongjun.ko, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(myeongjun.ko)

Sorry for the late reply. I've been on vacation.
This test case[0] also fails in the latest version.
I think that the test case requires debugging by line.

[0] browser_ext_themes_dynamic_getCurrent.js
https://dxr.mozilla.org/mozilla-central/rev/781f53bf9c789c27bc9d788a2b435c6304f03c88/toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_getCurrent.js

Flags: needinfo?(myeongjun.ko)

Shall I add a Check-in Needed tag? And should I take a look at the test case?
Or I'll look at the test case where the error occurs.

You should check why the test is failing, and update the patch.
Adding "Check-in Needed" without updating the patch would trigger the test failure again, and result in a backout.

Please look into the test failure, and feel free to ping me if you need more help with understanding the issue.

Comment on attachment 9107157 [details]
Bug 1585290 - Preserve theme from other extension upon theme.reset() r=robwu

Revision D52148 was moved to bug 1585289. Setting attachment 9107157 [details] to obsolete.

Attachment #9107157 - Attachment is obsolete: true

Thanks Rob.
I revised failed test code(i.e browser_ext_themes_dynamic_getCurrent.js). And I submitted patch again.
I gonna attach "Check-in Needed" tag after reviewing.

Flags: needinfo?(rob)

You made a mistake while updating the patch summary, which caused the patch to move to an unrelated bug. Could you update the bug number of the patch to move it back?

Meanwhile I'll review your change.

Flags: needinfo?(rob)
Attachment #9107157 - Attachment description: Bug 1585290 - theme.reset clears window theme even if another extension had called r=robwu → Bug 1585289 - theme.reset clears window theme even if another extension had called r=robwu
Attachment #9107157 - Attachment is obsolete: false
Attachment #9107157 - Attachment description: Bug 1585289 - theme.reset clears window theme even if another extension had called r=robwu → Bug 1585290 - theme.reset clears window theme even if another extension had called r=robwu

Oh. Sorry.
I lacked an explanation for patches.
I added comment :)
Feel free to tell me if you have any opinion.

Flags: needinfo?(rob)
Flags: needinfo?(rob)
Attachment #9107157 - Attachment description: Bug 1585290 - theme.reset clears window theme even if another extension had called r=robwu → Bug 1585290 - Preserve theme from other extension upon theme.reset() r=robwu
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9f7b5b1e6ec
Preserve theme from other extension upon theme.reset() r=robwu
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/504c03744391
Preserve theme from other extension upon theme.reset() r=robwu

Relanded by the above comment.

Flags: needinfo?(myeongjun.ko)
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76

Thanks to everyone for helping me to solve this issue.

You need to log in before you can comment on or make changes to this bug.