Closed Bug 892240 Opened 6 years ago Closed 6 years ago

Work - NewUI - Add tests for trimming and formatting of the URI

Categories

(Firefox for Metro Graveyard :: App Bar, defect)

x86_64
Windows 8
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jwilde, Assigned: rsilveira)

References

Details

(Whiteboard: feature=work)

Attachments

(1 file, 3 obsolete files)

Currently, we're missing this
Depends on: 883390
Summary: Add tests for trimming and formatting of the URI → Work - NewUI - Add tests for trimming and formatting of the URI
Status: NEW → ASSIGNED
Probably want to wait on reviewing this until all of the but 883394 patches land, but just to get it up here.
Attachment #774999 - Flags: review?(jmathies)
Re-exporting. Sorry for the bugspam.
Attachment #774999 - Attachment is obsolete: true
Attachment #774999 - Flags: review?(jmathies)
Attachment #775001 - Flags: review?(jmathies)
Attached patch tests (obsolete) — Splinter Review
Unbitrotted and should apply against today's m-c.
Attachment #775001 - Attachment is obsolete: true
Attachment #775001 - Flags: review?(jmathies)
Attachment #776534 - Flags: review?(jmathies)
Comment on attachment 776534 [details] [diff] [review]
tests

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

Don't see any issue here and these run fine on my surface pro. Not sure about the urlbar.xml edits though, don't really know that code at all.

::: browser/metro/base/tests/mochitest/browser_urlbar_highlightURLs.js
@@ +47,5 @@
> +  setUp: setUp,
> +  tearDown: tearDown,
> +  run: function () {
> +    let testcases = [
> +      "<https://>mozilla.org",

slick!
Attachment #776534 - Flags: review?(jmathies) → review+
Comment on attachment 776534 [details] [diff] [review]
tests

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

Bringing sfoster on to look at the urlbar.xml stuff. :D
Attachment #776534 - Flags: review?(sfoster)
Comment on attachment 776534 [details] [diff] [review]
tests

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

Overall looks great. We talked in channel about the touch/copy behavior - I was not able to copy the full url when I browsed to a page and touched in the urlbar to select, long press on selection and then copy. 

Also, I ran the full testsuite and the test files seperately. In the full test suite run, I got a test failure caused by an "Already Loading" exception in browser_urlbar_trimURLs.js, but when I ran it again it didn't reproduce.
Attachment #776534 - Flags: review?(sfoster) → feedback+
Whiteboard: feature=work → feature=work [preview]
Whiteboard: feature=work [preview] → feature=work [preview-triage]
Whiteboard: feature=work [preview-triage] → feature=work
The touch/copy behavior was fixed in bug 894715. As for the "already loading" exception, it looks like we get that out of more tests than just these, and there may be an issue related to the proxy system in the test runner?

I won't have more cycles to work on this and see whether this is the case this summer, though.
Assignee: jwilde → nobody
Attached patch Tests v2Splinter Review
unbitrotting again. All passed on a couple of local runs. Created a try run:
https://tbpl.mozilla.org/?tree=Try&rev=675b1f5bb7d1

I didn't look into what the urlbar changes are doing though.
Assignee: nobody → rsilveira
Attachment #776534 - Attachment is obsolete: true
Attachment #801928 - Flags: review?(sfoster)
Comment on attachment 801928 [details] [diff] [review]
Tests v2

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

Looks good, has a green try run and worked locally on my build also.

::: browser/metro/base/content/bindings/urlbar.xml
@@ +432,5 @@
>        <!-- Entering editing mode -->
>  
> +      <handler event="focus" phase="capturing">
> +        <![CDATA[
> +          this.beginEditing();

Doesn't seem to hurt or help with bug 903866 FWIW, but keeps better track of state within the binding.
Attachment #801928 - Flags: review?(sfoster) → review+
https://hg.mozilla.org/mozilla-central/rev/150bd140c640
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
I think we should disable this test unless an easy fix for bug 915620 is apparent.
Depends on: 915620
I'm in the tests today, I'll take a quick look at 915620.
No longer depends on: 915620
You need to log in before you can comment on or make changes to this bug.