Closed Bug 1491252 Opened Last year Closed Last year

Port URL overflow handling from urlbarBindings.xml to UrlbarInput.jsm

Categories

(Firefox :: Address Bar, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 2 obsolete files)

See updateTextOverflow in urlbarBindings.xml.
Blocks: 1491249
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #9009880 - Flags: review?(adw)
Attached patch patch (obsolete) — Splinter Review
rebased
Attachment #9009880 - Attachment is obsolete: true
Attachment #9009880 - Flags: review?(adw)
Attachment #9010596 - Flags: review?(standard8)
Priority: P2 → P1
Comment on attachment 9010596 [details] [diff] [review]
patch

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

Looks good.

For future reference, this functionality is already covered by browser/base/content/test/urlbar/browser_urlOverflow.js which we'll have to get working before we can enable the new urlbar functionality.
Attachment #9010596 - Flags: review?(standard8) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b130810bc4
Port URL overflow handling from urlbarBindings.xml to UrlbarInput.jsm. r=standard8
(In reply to Noemi Erli[:noemi_erli] from comment #5)
> Backed out changeset a6b130810bc4 (Bug 1491252) for xpcshall failures in
> browser/components/urlbar/tests/unit/test_UrlbarInput_unit.js

ERROR Unexpected exception TypeError: this.inputField is undefined; can't access its "addEventListener" property at resource:///modules/UrlbarInput.jsm:79
UrlbarInput@resource:///modules/UrlbarInput.jsm:79:5

*sigh*

UrlbarInput.jsm is very UI focused, so in hindsight it was probably a bad idea to test this with an xpcshell test rather than a mochitest-browser one. For now, I'll just extend the hacks in the test to make it pass.
Flags: needinfo?(dao+bmo)
Attached patch patch v2Splinter Review
Attachment #9010596 - Attachment is obsolete: true
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f138b7d87e7f
Port URL overflow handling from urlbarBindings.xml to UrlbarInput.jsm. r=standard8
(In reply to Dão Gottwald [::dao] from comment #6)
> For now, I'll just extend the hacks in the test to make it pass.

Hacks, or proper external dependency stubbing? Let's make sure we're all on the same page re. this!
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/f138b7d87e7f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > For now, I'll just extend the hacks in the test to make it pass.
> 
> Hacks, or proper external dependency stubbing? Let's make sure we're all on
> the same page re. this!

I was also uncertain when I landed the patch that using xpcshell-test was the right thing to do, but I thought I'd try it anyway.

The problem with xpcshell is that we don't have any way to create fake elements or simulate the, that means we have to formulate our own, but that's likely to get harder as we extend the view components.

I think the tests are at the right level - we should generally be doing a lot more actual unit testing than what we do - however I think we should just move these to mochitests. We can then use real or fake elements as inputs, and stub the methods on those where we need to.

I'll file a bug and get this done today.
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.