920877 - make media fragment: -moz-resolution work for blob files

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Phoebe Chang [:phoebe]]

Attachment: WIP - -moz-resolution works ok

Comment 1

[Phoebe Chang [:phoebe]]

Comment on attachment 810348 [details]
WIP - -moz-resolution works ok

media fragment uri ref "#-moz-resolution" won't work with blob file.
e.g. <img id="img" src="blob:d2923c04-8fb1-4880-8ad1-5dbd533e447b#-moz-resolution=256,256"></img>
it won't be able to find the file because it uses the whole src string (containing #-moz-resolution ref) to find the relative blob object.
I've fixed it by parsing the aUri string before getting the blob information from gDataTable.
Since this is my first bug, can anyone provide me any comments or advice?

Comment 2

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 810348 [details]
WIP - -moz-resolution works ok

Looks good so far, Phoebe! We should bring in a content peer as a reviewer once this patch gets a little further along, but I'll be happy to review as well and provide any feedback I can.

Let me start my feedback with a nit. I find it's better if comments describe what the code is doing instead of what you, the programmer, are doing. So I'd rephrase "fixed for mediafragmentation" to something like "Remove any fragment identifier from the URI."

More importantly, I'd suggest changing the design a bit here. My concern is that there are several places in this code where we call gDataTable->Put or gDataTable->Get, and all of them probably need the fragment filtering you're adding in your patch. We don't want to duplicate the same code in each place, though. I'd suggest a two step approach:

1. Replace calls to gDataTable->Get with calls to a static inline function ("getFromDataTable", perhaps?) which would call gDataTable->Get internally. The same goes for gDataTable->Put and any other cases that need fragment filtering. (gDataTable->Remove looks like another one.)

2. Perform the fragment filtering in the new functions. This way you don't have to duplicate the code in many places.

You don't necessarily need to do things this way, of course; you may have some better ideas!

You'll also want to add a test for this bug.

Thanks for identifying this bug and for finding the fix! Please feel free to request further feedback or a review from me once you get the patch updated.

Comment 3

[Phoebe Chang [:phoebe]]

Attachment: WIP - Bug 920877_Part 1_Remove fragment identifier from the URI when query data

Thank you for your advice!
I've changed the design to make those calling for info by gDataTable->Get to get the result returned by GetDataInfo(aUri) instead, which remove fragment identifier before calling gDataTable->Get. This will solve the duplication problem in my previous patch.
I think that there is no need of fragment filtering for gDataTable->Put since its Uri is fragment identifier free. gDataTable->Remove may have the chance to contain fragment identifier so I've made a modification in the RemoveDataEntry function.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 811906 [details] [diff] [review]
WIP - Bug 920877_Part 1_Remove fragment identifier from the URI when query data

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

Looks good!

At this point I think we should get a content peer involved. If you haven't seen it, you can find an appropriate peer for your patch using this page:

https://wiki.mozilla.org/Modules/All

Search for the name of the directory you're working in and you'll usually find the appropriate module and a list of peers.

In this case, Kyle Huey (:khuey) may be a good choice, particularly since he's working out of the Taipei office right now and you can talk to him in person if need be. If you're ready, go ahead and request review from him.

Comment 5

[Jason Duell]

> At this point I think we should get a content peer involved.

Yeah, this bug doesn't really belong in Core:Networking, so it's not a necko peer you're looking for :)

Comment 6

[Phoebe Chang [:phoebe]]

Attachment: WIP - Bug 920877_Part 2_Reftest

add an reftest to test if blob url with -moz-resolution fragment identifier can get the correct file and render it properly.
I've create the blob from a dataUri which is hard coded in the html file, it works but I think there is a more delecate way to implement this, working it out.

Comment 7

[Phoebe Chang [:phoebe]]

Comment on attachment 811906 [details] [diff] [review]
WIP - Bug 920877_Part 1_Remove fragment identifier from the URI when query data

Asking for feedback and review on Comments 1 and 3.
may you work this out for me? thank:)

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 811906 [details] [diff] [review]
WIP - Bug 920877_Part 1_Remove fragment identifier from the URI when query data

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

I'm giving this an r- mostly because it's your first patch.  Once you have a bit more experience these are changes I would trust you to make after giving r+.

You've made a good start at a commit message.  I would make it a little more specific, perhaps something like "Remove fragment identifier in nsHostObjectProtocolHandler before matching the URI."  You should also add the reviewer to the end of the commit message (in this case, "r=khuey").

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +49,5 @@
>  nsHostObjectProtocolHandler::RemoveDataEntry(const nsACString& aUri)
>  {
>    if (gDataTable) {
> +    int32_t hashPos = aUri.FindChar('#');
> +    if(hashPos > 0){

Mozilla style is to put spaces around our ifs, so this would be 

if (hashPos > 0) {

Also, you can declare variables in an if statement, so maybe even

if (int32_t hashPos = ...)

and the if will test the value of hashPos.

@@ +54,5 @@
> +      gDataTable->Remove(StringHead(aUri, hashPos));
> +    }
> +    else{
> +      gDataTable->Remove(aUri);
> +    }

I think this is cleaner if you have one call to Remove instead of two, and just do aURI = StringHead ... in the if statement.

@@ +86,5 @@
>    return NS_OK;
>  }
>  
> +static DataInfo*
> +GetDataInfo(const nsACString& aUri)

Similar comments apply to this function.

@@ +127,5 @@
>    if (!gDataTable) {
>      return;
>    }
>  
> +  DataInfo* res = GetDataInfo(aUri);

I think this should continue to call Get directly.  The list of URIs on the document should never have fragment identifiers in them.  In fact, it would be nice to assert here that aURI does not have a fragment identifier.

Comment 9

[Phoebe Chang [:phoebe]]

Attachment: WIP - Bug 920877_Part 1_Remove fragment identifier in nsHostObjectProtocolHandler before matching the URI

Thanks for the advice and review.
I've modified the if condition coding style and made Traverse function intact.
Remove and Get are called once by passing aUriIgnoringRef, which is the aUri with fragment identifier removed.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 812959 [details] [diff] [review]
WIP - Bug 920877_Part 1_Remove fragment identifier in nsHostObjectProtocolHandler before matching the URI

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

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +48,5 @@
>  void
>  nsHostObjectProtocolHandler::RemoveDataEntry(const nsACString& aUri)
>  {
>    if (gDataTable) {
> +    nsCString aUriIgnoringRef;

Our convention is Gecko is to use aFoo for arguments, mFoo for member variables, and just foo for local variables, so this should just be uriIgnoringRef.

@@ +49,5 @@
>  nsHostObjectProtocolHandler::RemoveDataEntry(const nsACString& aUri)
>  {
>    if (gDataTable) {
> +    nsCString aUriIgnoringRef;
> +    if (int32_t hashPos = aUri.FindChar('#')) {

So I realize now that this doesn't actually work, because if FindChar doesn't find anything it returns -1.  This condition just checks if hashPos is non-zero, so it will evaluate to true if FindChar doesn't find anything.  So you should undo this and go back to the way it was originally.

Sorry about that :-/

@@ +54,5 @@
> +      aUriIgnoringRef = StringHead(aUri, hashPos);
> +    }
> +    else {
> +      aUriIgnoringRef = aUri;
> +    }  

You have extra whitespace at the end of this line.

@@ +95,5 @@
> +  }
> +
> +  DataInfo* res;
> +  nsCString aUriIgnoringRef;
> +  if (int32_t hashPos = aUri.FindChar('#')) {

Same comments here about aUriIgnoring ref and the if.

@@ +102,5 @@
> +  else {
> +    aUriIgnoringRef = aUri;
> +  }
> +  gDataTable->Get(aUriIgnoringRef, &res);
> +  

Here too.

Comment 11

[Phoebe Chang [:phoebe]]

Attachment: WIP - Bug 920877_Part 1_Remove fragment identifier in nsHostObjectProtocolHandler before matching the URI

I've changed the variable name to match the convention, also undo Findchar to the original version. Thanks for the review.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 816417 [details] [diff] [review]
WIP - Bug 920877_Part 1_Remove fragment identifier in nsHostObjectProtocolHandler before matching the URI

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

r=me

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 812499 [details] [diff] [review]
WIP - Bug 920877_Part 2_Reftest

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

You can start the review process for this test case. :)

::: content/base/test/reftest/reftest.list
@@ +1,1 @@
> +== test_bug920877.html  test_bug920877-ref.html 

nit: remove tailing space.

::: content/base/test/reftest/test_bug920877-ref.html
@@ +1,1 @@
> +<body>

Is <html> missing in the begin of this file?

::: content/base/test/reftest/test_bug920877.html
@@ +11,5 @@
> +
> +var data = atob(dataURL.substring( "data:image/vnd.microsoft.icon;base64,".length ) );
> +var asArray = new Uint8Array(data.length);
> +for( var i = 0, len = data.length; i < len; ++i ) {
> +    asArray[i] = data.charCodeAt(i);    

nit: remove tailing space.

Comment 14

[Phoebe Chang [:phoebe]]

Attachment: WIP - Bug 920877_Part 2_Reftest

A reftest is created to test if blob url with -moz-resolution fragment identifier can get the correct file (mixed-bmp-png.ico) and render it properly. 
Also added the reftest entry to /content/test/reftest/reftest.list

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 816426 [details] [diff] [review]
WIP - Bug 920877_Part 2_Reftest

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

r=me

::: content/base/test/reftest/test_bug920877.html
@@ +30,5 @@
> +img.id = "img-res48";
> +img.src = url + '#-moz-resolution=48,48';
> +document.body.appendChild(img);
> +
> +window.URL.revokeObjectURL(url);

Drop 'window.' here.

::: content/test/reftest/reftest.list
@@ +2,5 @@
>  # From: /content/test/reftest
>  # To:   /content/canvas/test/reftest
>  skip-if(xulFennec) include ../../canvas/test/reftest/reftest.list
>  
> +include ../../base/test/reftest/reftest.list # bug 920877

No need for the comment here.

Comment 16

[Phoebe Chang [:phoebe]]

https://tbpl.mozilla.org/?tree=Try&rev=c5247d401416

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8714cdf381
https://hg.mozilla.org/integration/mozilla-inbound/rev/112e8748b75c

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae8714cdf381
https://hg.mozilla.org/mozilla-central/rev/112e8748b75c