Closed Bug 1402399 Opened 4 years ago Closed 3 years ago

Cannot clear titlePreface by calling browser.windows.update with empty string as titlePreface

Categories

(WebExtensions :: Frontend, enhancement, P5)

57 Branch
enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: robwu, Assigned: tushararora, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

browser.windows.update(-2, {titlePreface: 'xxx'}); // sets title preface to 'xxx'
browser.windows.update(-2, {titlePreface: ''});
// Expected: Cleared title preface.
// Actual  : Title preface is still at 'xxx'.

https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/browser/components/extensions/ext-windows.js#190 and
https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/browser/components/extensions/ext-windows.js#221

should be changed to use: createData.titlePreface !== null
Mentor: bob.silverberg
Keywords: good-first-bug
Priority: -- → P5
Hey! I would like to work on this as my first bug.
Flags: needinfo?(bob.silverberg)
(In reply to Tushar Arora from comment #1)
> Hey! I would like to work on this as my first bug.

That's great Tushar, thanks for your interest.

If you haven't built Firefox before, that's the place to start. The simplest way to do that is explained here:

https://developer.mozilla.org/en-US/docs/Artifact_builds

Once you have a working build, you can make the changes to the code as described in the bug description above. We will also need an existing test to be updated to prove that this change works. The test in question can be found at http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/components/extensions/test/browser/browser_ext_windows.js.

Let me know if you run into any problems or have any questions.
Flags: needinfo?(bob.silverberg)
Attached patch title-preface-fix.patch (obsolete) — Splinter Review
Attaching the patch, please review.
Attachment #8913314 - Flags: review?(bob.silverberg)
Comment on attachment 8913314 [details] [diff] [review]
title-preface-fix.patch

Review of attachment 8913314 [details] [diff] [review]:
-----------------------------------------------------------------

Great work, Tushar, thank you for the patch.

Everything in here looks excellent, but looking at the code now I realize that we do not have test coverage for titlePreface being null. I.e., we do not have a test where we set a titlePreface and then call windows.update and pass nothing into the titlePreface property of the updateInfo object, and then assert that the preface has not changed. Do you mind updating the patch to add a test for that case as well?
Assignee: nobody → tushararora.cs
Hey, thanks Bob! Do you think instead of using !== we should use != to check for undefined as well as null?
Flags: needinfo?(bob.silverberg)
(In reply to Tushar Arora from comment #5)
> Hey, thanks Bob! Do you think instead of using !== we should use != to check
> for undefined as well as null?

No, I think it's fine to use !== null, because the framework will pass a null if nothing is passed into the options object. This will need to go to a final review by a peer, so that may be wrong, but for now I think it's fine.
Flags: needinfo?(bob.silverberg)
Comment on attachment 8913314 [details] [diff] [review]
title-preface-fix.patch

Review of attachment 8913314 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing my review flag for now so you can re-add it when the patch is ready for another review.
Attachment #8913314 - Flags: review?(bob.silverberg)
Updated the patch with one more test for empty titlePreface. Please review.
Attachment #8913314 - Attachment is obsolete: true
Attachment #8918178 - Flags: review?(bob.silverberg)
Comment on attachment 8918178 [details] [diff] [review]
title-preface-fix-updated-tests.patch

Review of attachment 8918178 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks Tushar. I will flag one of the WebExtensions peers for a final review.
Attachment #8918178 - Flags: review?(mixedpuppy)
Attachment #8918178 - Flags: review?(bob.silverberg)
Attachment #8918178 - Flags: review+
Attachment #8918178 - Flags: review?(mixedpuppy) → review+
Shane, setting check-in needed, we assume this is ready to land, given the r+.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad69d5466d3c
Fix browser.windows.update to clear titlePreface if it is passed empty. r=bsilverberg, r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ad69d5466d3c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(tushararora.cs)
Flags: needinfo?(tushararora.cs) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.