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

RESOLVED FIXED in Firefox 59

Status

enhancement
P5
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: robwu, Assigned: tushararora, Mentored)

Tracking

({good-first-bug})

57 Branch
mozilla59
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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

Updated

2 years ago
Mentor: bob.silverberg
Keywords: good-first-bug
Priority: -- → P5
Assignee

Comment 1

2 years ago
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)
Assignee

Comment 3

2 years ago
Posted 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
Assignee

Comment 5

2 years ago
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)
Assignee

Comment 8

2 years ago
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

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ad69d5466d3c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 13

2 years ago
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-

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.