1126362 - Add unit tests for navigating to a data: url containing an image

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[James Graham [:jgraham]]

If I (using the python client) navigate to a data: url containing a base64 encoded png image the navigate call never returns.

Comment 1

[Andreas Tolfsen ❲:ato❳]

I believe this was solved by :whimboo’s excellent navigation work.  Letting him confirm that.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It works but we don't have a test for that. Given that all of our navigation tests are under Marionette harness unit tests we should add one there for now:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py

The following test loads a 1px x 1px PNG file:

> def test(self):
>     self.marionette.navigate("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==")

We would need as best a test for the bfcache class, so we can verify `back()` and `forward()` at the same time.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Bumping to P2 then as that is conformance work.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mike, would you be interested on this bug?

Comment 5

[Mike Yusko(:iceman)]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Mike, would you be interested on this bug?

Hey Henrik, yes I'll work on the bug, could you provide a little bit more details? Thank you.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mike, please see my comment 2. I think that all the necessary information is there already.

Comment 7

[Mike Yusko(:iceman)]

Attachment: Bug 1126362 - Add unit tests for navigating to a data: url containing an image

Review commit: https://reviewboard.mozilla.org/r/223144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223144/

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

https://reviewboard.mozilla.org/r/223144/#review229472

Thank you Mike for that first patch. It's good to see that you were able to figure out all the necessary changes yourself. Nevertheless I have a couple of comments which would need to be addressed. Once that has been done I will trigger a try build. Thank you!

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:29
(Diff revision 1)
>  
>  def inline(doc):
>      return "data:text/html;charset=utf-8,%s" % urllib.quote(doc)
>  
> +def inline_image(base64_img):
> +    formatted_url = base64_img.replace('\n', '').replace(' ', '')

Lets kill this line by having the image data in a single line, as mentioned below.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:30
(Diff revision 1)
>  def inline(doc):
>      return "data:text/html;charset=utf-8,%s" % urllib.quote(doc)
>  
> +def inline_image(base64_img):
> +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> +    base64_url = 'data:image/png;base64,' + formatted_url

Strings shouldn't be concatenated by using `+`, but via a formatting like `%s` or `.format()`. See the `inline()` method above.

Also the function argument can just be named `data`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:31
(Diff revision 1)
>      return "data:text/html;charset=utf-8,%s" % urllib.quote(doc)
>  
> +def inline_image(base64_img):
> +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> +    base64_url = 'data:image/png;base64,' + formatted_url
> +    return base64_url

There is no need to have this extra variable. You can directly return the computated value.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:33
(Diff revision 1)
> +def inline_image(base64_img):
> +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> +    base64_url = 'data:image/png;base64,' + formatted_url
> +    return base64_url
> +
> +FIRST_BASE64_IMAGE = """

What kind of image is that? Size, color, etc?

Please put all base64 encoded data into a single line. If the linter complains we should add maybe #noqa to that line.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:39
(Diff revision 1)
> +    iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAY
> +    AAAAfFcSJAAAADUlEQVR42mNkYPhfDwACh
> +    wGA60e6kgAAAABJRU5ErkJggg==
> +"""
> +
> +SECOND_BASE64_IMAGE = """

Same question as above for the other image. Lets make the name a bit more clear. 

Also please try to reduce this base64 encoded data so that it has a similar lenght like the first image.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:529
(Diff revision 1)
>          test_pages = [
>              {"url": self.marionette.absolute_url("black.png")},
>              {"url": self.marionette.absolute_url("white.png")},
>          ]
>          self.run_bfcache_test(test_pages)
> +    

Please note these red blocks as shown in mozreview. The linter should have complained about those. Did you ran `mach lint testing/marionette`?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:531
(Diff revision 1)
>              {"url": self.marionette.absolute_url("white.png")},
>          ]
>          self.run_bfcache_test(test_pages)
> +    
>  
> +    def test_image_urls(self):

To be inline with the naming of other tests lets use `test_data_images`, and put the test close to `test_data_urls`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:532
(Diff revision 1)
> +        image_urls = [
> +            {"url": inline_image(FIRST_BASE64_IMAGE)},

A test like that tests that the navigation works fine between data images, which is one part. What I would like to see too is a navigation to a normal image. Something which "test_data_url" does for a HTML page. Please add that in the middle of this list.

Comment 9

[Mike Yusko(:iceman)]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 8953989 [details]
> Bug 1126362 - Add unit tests for navigating to a data: url containing an
> image
> 
> https://reviewboard.mozilla.org/r/223144/#review229472
> 
> Thank you Mike for that first patch. It's good to see that you were able to
> figure out all the necessary changes yourself. Nevertheless I have a couple
> of comments which would need to be addressed. Once that has been done I will
> trigger a try build. Thank you!
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 29
> (Diff revision 1)
> >  
> >  def inline(doc):
> >      return "data:text/html;charset=utf-8,%s" % urllib.quote(doc)
> >  
> > +def inline_image(base64_img):
> > +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> 
> Lets kill this line by having the image data in a single line, as mentioned
> below.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 30
> (Diff revision 1)
> >  def inline(doc):
> >      return "data:text/html;charset=utf-8,%s" % urllib.quote(doc)
> >  
> > +def inline_image(base64_img):
> > +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> > +    base64_url = 'data:image/png;base64,' + formatted_url
> 
> Strings shouldn't be concatenated by using `+`, but via a formatting like
> `%s` or `.format()`. See the `inline()` method above.
> 
> Also the function argument can just be named `data`.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 31
> (Diff revision 1)
> >      return "data:text/html;charset=utf-8,%s" % urllib.quote(doc)
> >  
> > +def inline_image(base64_img):
> > +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> > +    base64_url = 'data:image/png;base64,' + formatted_url
> > +    return base64_url
> 
> There is no need to have this extra variable. You can directly return the
> computated value.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 33
> (Diff revision 1)
> > +def inline_image(base64_img):
> > +    formatted_url = base64_img.replace('\n', '').replace(' ', '')
> > +    base64_url = 'data:image/png;base64,' + formatted_url
> > +    return base64_url
> > +
> > +FIRST_BASE64_IMAGE = """
> 
> What kind of image is that? Size, color, etc?
> 
> Please put all base64 encoded data into a single line. If the linter
> complains we should add maybe #noqa to that line.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 39
> (Diff revision 1)
> > +    iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAY
> > +    AAAAfFcSJAAAADUlEQVR42mNkYPhfDwACh
> > +    wGA60e6kgAAAABJRU5ErkJggg==
> > +"""
> > +
> > +SECOND_BASE64_IMAGE = """
> 
> Same question as above for the other image. Lets make the name a bit more
> clear. 
> 
> Also please try to reduce this base64 encoded data so that it has a similar
> lenght like the first image.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 529
> (Diff revision 1)
> >          test_pages = [
> >              {"url": self.marionette.absolute_url("black.png")},
> >              {"url": self.marionette.absolute_url("white.png")},
> >          ]
> >          self.run_bfcache_test(test_pages)
> > +    
> 
> Please note these red blocks as shown in mozreview. The linter should have
> complained about those. Did you ran `mach lint testing/marionette`?
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 531
> (Diff revision 1)
> >              {"url": self.marionette.absolute_url("white.png")},
> >          ]
> >          self.run_bfcache_test(test_pages)
> > +    
> >  
> > +    def test_image_urls(self):
> 
> To be inline with the naming of other tests lets use `test_data_images`, and
> put the test close to `test_data_urls`.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:
> 532
> (Diff revision 1)
> > +        image_urls = [
> > +            {"url": inline_image(FIRST_BASE64_IMAGE)},
> 
> A test like that tests that the navigation works fine between data images,
> which is one part. What I would like to see too is a navigation to a normal
> image. Something which "test_data_url" does for a HTML page. Please add that
> in the middle of this list.

Thank you for the review, I'll fix all your comments today, or tomorrow. Thanks!

Comment 10

[Mike Yusko(:iceman)]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223144/diff/1-2/

Comment 11

[Mike Yusko(:iceman)]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223144/diff/2-3/

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

https://reviewboard.mozilla.org/r/223144/#review231218

::: commit-message-66e87:6
(Diff revision 3)
> +Bug 1126362 - Add unit tests for navigating to a data: url containing an image r?whimboo
> +
> +MozReview-Commit-ID: GZg1BOY7tOj
> +
> +Bug 1126362 - Add unit tests for navigating to a data: url containing an image r?whimboo
> +Bug 1126362 - Add unit tests for navigating to a data: url containing an image, also remove tralling spaces r?whimboo

When you commit or run a histedit make sure to remove those extra commit messages at the bottom.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:25
(Diff revision 3)
>  )
>  
>  here = os.path.abspath(os.path.dirname(__file__))
>  
>  
> +BLACK_IMAGE_PX1 = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==' # noqa

I would suggest to name the variables like `BLACK_PIXEL` and `RED_PIXEL`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:414
(Diff revision 3)
>              {"url": inline("<p>foobar</p>")},
>          ]
>          self.run_bfcache_test(test_pages)
>  
> +    def test_data_url(self):
> +        self.marionette.navigate(inline_image(BLACK_IMAGE_PX1))

This is not a valid bfcache test because it doesn't setup test_pages. Please remove this test also because it is testing what you already added in test_image_urls.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:419
(Diff revision 3)
> +        self.marionette.navigate(inline_image(BLACK_IMAGE_PX1))
> +
> +    def test_image_urls(self):
> +        image_urls = [
> +            {"url": inline_image(BLACK_IMAGE_PX1)},
> +            {"url": inline_image(RED_IMAGE_PX1)}

Please make sure that we test the following states:

- normal image
- data image
- data image
- normal image

And as mentioned the last time please move the test close to test_navigate_to_same_image_document_twice, or better extend the test with the above additional states.

Comment 13

[Mike Yusko(:iceman)]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223144/diff/3-4/

Comment 14

[Mike Yusko(:iceman)]

Attachment: Bug 1126362 - Fix comments

Bug 1126362 - Fix comments r?whimboo


Bug 1126362 - Fix r? whimboo
Bug 1126362 - Fix tralling spaces r? whimboo
Bug 1126362 - Remove tralling spaces r? whimboo

Review commit: https://reviewboard.mozilla.org/r/225714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225714/

Comment 15

[Mike Yusko(:iceman)]

Hello Henrik, I added the additional commit, and sorry that I did't `--amend`, I'm a little bit was confused with mercurial.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Mike Yusko(:iceman) from comment #15)
> Hello Henrik, I added the additional commit, and sorry that I did't
> `--amend`, I'm a little bit was confused with mercurial.

Please always amend fixes for review comments. Otherwise you won't get a r+. Also it's not necessary to set an additional needinfo flag when the review flag is set. I will see it in my inbox. Thanks.

Comment 17

[Mike Yusko(:iceman)]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> (In reply to Mike Yusko(:iceman) from comment #15)
> > Hello Henrik, I added the additional commit, and sorry that I did't
> > `--amend`, I'm a little bit was confused with mercurial.
> 
> Please always amend fixes for review comments. Otherwise you won't get a r+.
> Also it's not necessary to set an additional needinfo flag when the review
> flag is set. I will see it in my inbox. Thanks.

Yes Henrik, I know it, you mentioned concerning this case, a few weeks ago. I was amended a commits, but I didn't see their in my first commit. So, it's was forced step, sorry;(

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

https://reviewboard.mozilla.org/r/223144/#review231616

::: commit-message-66e87
(Diff revisions 3 - 4)
>  Bug 1126362 - Add unit tests for navigating to a data: url containing an image r?whimboo
>  
>  MozReview-Commit-ID: GZg1BOY7tOj
>  
>  Bug 1126362 - Add unit tests for navigating to a data: url containing an image r?whimboo
> -Bug 1126362 - Add unit tests for navigating to a data: url containing an image, also remove tralling spaces r?whimboo

Please remove that line.

I would suggest that you check your uploaded patch before publishing it. It should only contain the appropriate changes. By having a look at the mozreview patch preview (click the last link when submitting a patch), you would have seen all this.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:499
(Diff revisions 3 - 4)
>          test_pages = [
>              {"url": self.marionette.absolute_url("black.png")},
>              {"url": self.marionette.absolute_url("white.png")},
>          ]
>          self.run_bfcache_test(test_pages)
> +      

There are white-spaces again, which should have caused linter failures. Please remember to always run the test after a modification, and to use the linter before submission.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8956750 [details]
Bug 1126362 - Fix comments

https://reviewboard.mozilla.org/r/225714/#review231618

This patch needs to be amended.

Comment 20

[Mike Yusko(:iceman)]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223144/diff/4-5/

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

https://reviewboard.mozilla.org/r/223144/#review232270

Mike, this looks way better now. Thank you for the update! There are three issues remaining to fix. Once that is done I can push the patch to try, and we can see how it works across platforms.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:25
(Diff revision 5)
>  )
>  
>  here = os.path.abspath(os.path.dirname(__file__))
>  
>  
> +BLACK_PIXEL= 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==' # noqa

nit: please add spaces around `=`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:413
(Diff revision 5)
>              {"url": self.test_page_remote},
>              {"url": inline("<p>foobar</p>")},
>          ]
>          self.run_bfcache_test(test_pages)
>  
> +

This added extra line should cause again a linter failure.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:500
(Diff revision 5)
>              {"url": self.marionette.absolute_url("black.png")},
>              {"url": self.marionette.absolute_url("white.png")},
>          ]
>          self.run_bfcache_test(test_pages)
>  
> +    def test_image_urls(self):

You want to add the content of this test to `test_image_to_image`. Otherwise we would have duplication now.

Comment 22

[Mike Yusko(:iceman)]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223144/diff/5-6/

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The test looks fine to me now. I pushed a try build and will finally review once the tests in CI are done.

Comment 24

[Mike Yusko(:iceman)]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> The test looks fine to me now. I pushed a try build and will finally review
> once the tests in CI are done.

Nice!

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8953989 [details]
Bug 1126362 - Add unit tests for navigating to a data: url containing an image

https://reviewboard.mozilla.org/r/223144/#review233256

Try job is green. So we can get this landed. Thank you Mike!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a8f38951454
Add unit tests for navigating to a data: url containing an image r=whimboo

Comment 27

[Mike Yusko(:iceman)]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 8953989 [details]
> Bug 1126362 - Add unit tests for navigating to a data: url containing an
> image
> 
> https://reviewboard.mozilla.org/r/223144/#review233256
> 
> Try job is green. So we can get this landed. Thank you Mike!

Np;) I'll see you via IRC, and maybe we will find a new bugs for me.

Thank you Henrik for the reviews!

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5a8f38951454