Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2105283002:
<image>'s src URL is resolved incorrectly when <base> element dynamically added (Closed)
|
Created:
4 years, 5 months ago by kyounga.ra Modified:
4 years, 5 months ago Reviewers:
Yoav Weiss CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description<image>'s src URL is resolved incorrectly when <base> element dynamically added.
<image>'s src URL should not be affected by <base> element which is added later.
BUG=569760
R=yoav@yoav.ws
Committed: https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1
Cr-Commit-Position: refs/heads/master@{#406447}
Patch Set 1 #Patch Set 2 : <image>'s src URL is resolved incorrectly when <base> element dynamically added. #
Total comments: 6
Patch Set 3 : <image>'s src URL is resolved incorrectly when <base> element dynamically added. #
Total comments: 3
Patch Set 4 : <image>'s src URL is resolved incorrectly when <base> element dynamically added. #
Total comments: 8
Patch Set 5 : <image>'s src URL is resolved incorrectly when <base> element dynamically added. #
Messages
Total messages: 35 (10 generated)
Description was changed from ========== <image>'s src URL is resolved incorrecty when <base> element dynamically added BUG=569760 R=cbiesinger@chromium.org ========== to ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=cbiesinger@chromium.org ==========
kyounga.ra@gmail.com changed reviewers: - cbiesinger@chromium.org
kyounga.ra@gmail.com changed reviewers: + yoav@yoav.ws
I uploaded a small patch for bug #569760. It's small and will not take much time. any comments and ideas are welcome.
On 2016/06/30 02:06:56, kyounga.ra wrote: > I uploaded a small patch for bug #569760. > > It's small and will not take much time. > any comments and ideas are welcome. Hey Kyounga, Thanks for working on this! Could provide a test that shows what this change is doing and makes sure that it won't regress? Could you also run the trybots (`git cl try`) in order to see that it is not regressing any of the current tested behaviors?
On 2016/06/30 09:42:48, Yoav Weiss wrote: > On 2016/06/30 02:06:56, kyounga.ra wrote: > > I uploaded a small patch for bug #569760. > > > > It's small and will not take much time. > > any comments and ideas are welcome. > > Hey Kyounga, > > Thanks for working on this! Could provide a test that shows what this change is > doing and makes sure that it won't regress? Could you also run the trybots (`git > cl try`) in order to see that it is not regressing any of the current tested > behaviors? test file has attached on bug=569760. https://bugs.chromium.org/p/chromium/issues/detail?id=569760 I just tried "git cl try" and got an error msg. (ERROR: Access denied: cannot add builds to bucket master.tryserver.chromium.android) Is there somthing I should to do? Thanks for your help.
On 2016/06/30 23:36:40, kyounga.ra wrote: > On 2016/06/30 09:42:48, Yoav Weiss wrote: > > On 2016/06/30 02:06:56, kyounga.ra wrote: > > > I uploaded a small patch for bug #569760. > > > > > > It's small and will not take much time. > > > any comments and ideas are welcome. > > > > Hey Kyounga, > > > > Thanks for working on this! Could provide a test that shows what this change > is > > doing and makes sure that it won't regress? Could you also run the trybots > (`git > > cl try`) in order to see that it is not regressing any of the current tested > > behaviors? > > test file has attached on bug=569760. > https://bugs.chromium.org/p/chromium/issues/detail?id=569760 Could you add a similar test case as a layout test? https://www.chromium.org/developers/testing/webkit-layout-tests > > I just tried "git cl try" and got an error msg. (ERROR: Access denied: cannot > add builds to bucket master.tryserver.chromium.android) > Is there somthing I should to do? I guess you're lacking trybot permissions. For now I've run some of them for you. > > Thanks for your help.
On 2016/07/01 06:44:14, Yoav Weiss wrote: > On 2016/06/30 23:36:40, kyounga.ra wrote: > > On 2016/06/30 09:42:48, Yoav Weiss wrote: > > > On 2016/06/30 02:06:56, kyounga.ra wrote: > > > > I uploaded a small patch for bug #569760. > > > > > > > > It's small and will not take much time. > > > > any comments and ideas are welcome. > > > > > > Hey Kyounga, > > > > > > Thanks for working on this! Could provide a test that shows what this change > > is > > > doing and makes sure that it won't regress? Could you also run the trybots > > (`git > > > cl try`) in order to see that it is not regressing any of the current tested > > > behaviors? > > > > test file has attached on bug=569760. > > https://bugs.chromium.org/p/chromium/issues/detail?id=569760 > > Could you add a similar test case as a layout test? > https://www.chromium.org/developers/testing/webkit-layout-tests > I uploaded the new patch-set(#2) including layout-test. > > > > > I just tried "git cl try" and got an error msg. (ERROR: Access denied: cannot > > add builds to bucket master.tryserver.chromium.android) > > Is there somthing I should to do? > > I guess you're lacking trybot permissions. For now I've run some of them for > you. > > > > > Thanks for your help.
Thanks for working on this! :) A few comments regarding test style and compatibility. Otherwise an explanation regarding your solution and why it's working would be helpful when reviewing. https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:10: var image = document.getElementsByTagName('img')[1]; I'm assuming you're using natural{Width, Height} as a proxy to the fact that the image is broken? Could you use onerror instead, which is more direct? Could you use testharness based tests, instead of an expected.txt one? (Tutorial at http://testthewebforward.org/docs/testharness-tutorial.html and you can grep "testharness" for examples layout tests) https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:20: <div id=""> Why a div here? https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:21: <img id="img1" src=""><br> Is the empty "src" part of the test? If not, can you eliminate it? https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:22: Reference<br> Why is this indented? https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:26: document.getElementById('ref').src = 'resources/image1.png'; Inconsistent indentation. 4 spaces as indentation is the common style in Blink. https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/ImageLoader.cpp:276: AtomicString imageSourceURL = m_element->imageSourceURL(); Can you explain why moving the imageSourceToKURL call to the time the task is created is helpful? What is the value of imageSourceURL here and how does it differ from url? If url is the correct one here, we should probably deduce imageSourceURL from it, rather than grabbing the value from m_element
Oops, sorry for my test page, with inattention. On 2016/07/04 07:18:14, Yoav Weiss wrote: > Thanks for working on this! :) > > A few comments regarding test style and compatibility. Otherwise an explanation > regarding your solution and why it's working would be helpful when reviewing. > > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): > > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/loader/image-loader-base.html:10: var image = > document.getElementsByTagName('img')[1]; > I'm assuming you're using natural{Width, Height} as a proxy to the fact that the > image is broken? > Could you use onerror instead, which is more direct? > Modified. > Could you use testharness based tests, instead of an expected.txt one? (Tutorial > at http://testthewebforward.org/docs/testharness-tutorial.html and you can grep > "testharness" for examples layout tests) > Modified, I'm not familiar with testharness based tests but make it simple. > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/loader/image-loader-base.html:20: <div id=""> > Why a div here? > Deleted. > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/loader/image-loader-base.html:21: <img id="img1" > src=""><br> > Is the empty "src" part of the test? If not, can you eliminate it? > Deleted. > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/loader/image-loader-base.html:22: Reference<br> > Why is this indented? > Modified. > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/loader/image-loader-base.html:26: > document.getElementById('ref').src = 'resources/image1.png'; > Inconsistent indentation. 4 spaces as indentation is the common style in Blink. > Modified. > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/2105283002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/ImageLoader.cpp:276: AtomicString > imageSourceURL = m_element->imageSourceURL(); > Can you explain why moving the imageSourceToKURL call to the time the task is > created is helpful? What is the value of imageSourceURL here and how does it > differ from url? > > If url is the correct one here, we should probably deduce imageSourceURL from > it, rather than grabbing the value from m_element The problem is not to display second <img> element. The second <img>'s "src" attribute was set "resources/image1.png" before <base href="resources/"> element was added. We expected the resolved URL as "path_from_root/resources/image1.png". (There is no <base> at that time, when the "src" was set) But, it's resolved as "path_from_root/resources/resources/image1.png" here. (<base> is added. ) So, I moved the imageSourceToKURL() call to the time the task is created. (<base> was not added yet.) imageSourceURL is just "src" attribute value, not resolved URL. ("resources/image1.png")
Sorry for the delay. Feel free to ping me here or on IRC if I fail to review new patchsets within 24-48H. https://codereview.chromium.org/2105283002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): https://codereview.chromium.org/2105283002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:9: test (function() { You want to run an async test which is initiated globally. See LayoutTests/http/tests/preload for examples. https://codereview.chromium.org/2105283002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:16: assert_true(true, "reference image is loadec successfully"); s/loadec/loaded/ https://codereview.chromium.org/2105283002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:29: ref.src = 'resources/image1.png'; You're testing the reference image rather than the one you actually want to test. It might be better to get rid of the reference image here altogether
uploaded patch-set #4
uploaded patch-set #4
Hello, Yoav Could you have a time for this? Thanks in advance,
https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:16: elm.src = 'resources/image1.png'; shouldn't that say "image1.png"?
On 2016/07/19 06:15:17, Yoav Weiss wrote: > https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): > > https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/loader/image-loader-base.html:16: elm.src = > 'resources/image1.png'; > shouldn't that say "image1.png"? I just wanted to reuse the existed image. I don't need to upload a new image file because don't need a special image.
On 2016/07/19 06:24:39, kyounga.ra wrote: > On 2016/07/19 06:15:17, Yoav Weiss wrote: > > > https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): > > > > > https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/loader/image-loader-base.html:16: elm.src = > > 'resources/image1.png'; > > shouldn't that say "image1.png"? > > I just wanted to reuse the existed image. > I don't need to upload a new image file because don't need a special image. sure, but you want to test that the image take the <base> into account, so want that URL to be relative to the base, right?
On 2016/07/19 06:33:29, Yoav Weiss wrote: > On 2016/07/19 06:24:39, kyounga.ra wrote: > > On 2016/07/19 06:15:17, Yoav Weiss wrote: > > > > > > https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... > > > File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): > > > > > > > > > https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/loader/image-loader-base.html:16: elm.src = > > > 'resources/image1.png'; > > > shouldn't that say "image1.png"? > > > > I just wanted to reuse the existed image. > > I don't need to upload a new image file because don't need a special image. > > sure, but you want to test that the image take the <base> into account, so want > that URL to be relative to the base, right? Ah~ it's my understanding. I'm testing if the image does NOT to take the <base> into account. because <img>'s "src" attribute is set when the <base> is NOT exist. So, resolved image path should be "~path/resources/image1.png" but it is "~path/resources/resources/image1.png" in Chrome browser only
LGTM % nits. Please fix the tiny style issues before committing. Thanks for working on this! :) https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:16: elm.src = 'resources/image1.png'; On 2016/07/19 06:15:17, Yoav Weiss wrote: > shouldn't that say "image1.png"? OK, I somehow thought the issue is the inverse (probably from the fact that I now see that the problem in the initial test case is seen on the "ref" image) https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:18: assert_unreached(" onerror() of image."); spurious " " at beginning of sentence https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:22: assert_true(true, " Loaded successfully."); spurious " " at beginning of sentence https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/ImageLoader.cpp:111: m_requestURL = loader->imageSourceToKURL(imageSourceURL); Can you put all that in a single line? so something like "m_requestURL = loader->imageSourceToKURL(loader->element()->imageSourceURL());"
The CQ bit was checked by kyounga.ra@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws Link to the patchset: https://codereview.chromium.org/2105283002/#ps80001 (title: "<image>'s src URL is resolved incorrectly when <base> element dynamically added.")
The CQ bit was unchecked by kyounga.ra@gmail.com
The CQ bit was checked by kyounga.ra@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/loader/image-loader-base.html (right): https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:18: assert_unreached(" onerror() of image."); On 2016/07/19 09:15:29, Yoav Weiss wrote: > spurious " " at beginning of sentence Done. https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/loader/image-loader-base.html:22: assert_true(true, " Loaded successfully."); On 2016/07/19 09:15:29, Yoav Weiss wrote: > spurious " " at beginning of sentence Done. https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2105283002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/ImageLoader.cpp:111: m_requestURL = loader->imageSourceToKURL(imageSourceURL); On 2016/07/19 09:15:29, Yoav Weiss wrote: > Can you put all that in a single line? so something like "m_requestURL = > loader->imageSourceToKURL(loader->element()->imageSourceURL());" Done.
Description was changed from ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=cbiesinger@chromium.org ========== to ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=yoav@yoav.ws ==========
Message was sent while issue was closed.
Description was changed from ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=yoav@yoav.ws ========== to ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=yoav@yoav.ws ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=yoav@yoav.ws ========== to ========== <image>'s src URL is resolved incorrectly when <base> element dynamically added. <image>'s src URL should not be affected by <base> element which is added later. BUG=569760 R=yoav@yoav.ws Committed: https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1 Cr-Commit-Position: refs/heads/master@{#406447} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1 Cr-Commit-Position: refs/heads/master@{#406447}
Message was sent while issue was closed.
On 2016/07/20 01:44:36, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1 > Cr-Commit-Position: refs/heads/master@{#406447} Congrats on your first committed patch :)
Message was sent while issue was closed.
On 2016/07/20 08:51:11, Yoav Weiss wrote: > On 2016/07/20 01:44:36, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1 > > Cr-Commit-Position: refs/heads/master@{#406447} > > Congrats on your first committed patch :) One (glaring) omission on my part: you need to sign the contributor agreement as described in https://www.chromium.org/developers/contributing-code
Message was sent while issue was closed.
On 2016/07/20 at 09:03:55, yoav wrote: > On 2016/07/20 08:51:11, Yoav Weiss wrote: > > On 2016/07/20 01:44:36, commit-bot: I haz the power wrote: > > > Patchset 5 (id:??) landed as > > > https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1 > > > Cr-Commit-Position: refs/heads/master@{#406447} > > > > Congrats on your first committed patch :) > > One (glaring) omission on my part: you need to sign the contributor agreement as described in https://www.chromium.org/developers/contributing-code I checked and they did sign it. In the future, please first ask a googler to verify that they signed the CLA :)
Message was sent while issue was closed.
On 2016/07/21 07:48:09, jochen wrote: > On 2016/07/20 at 09:03:55, yoav wrote: > > On 2016/07/20 08:51:11, Yoav Weiss wrote: > > > On 2016/07/20 01:44:36, commit-bot: I haz the power wrote: > > > > Patchset 5 (id:??) landed as > > > > https://crrev.com/d467d3b7bdbe87694a7d31c2b73ae0546a4701b1 > > > > Cr-Commit-Position: refs/heads/master@{#406447} > > > > > > Congrats on your first committed patch :) > > > > One (glaring) omission on my part: you need to sign the contributor agreement > as described in https://www.chromium.org/developers/contributing-code > > I checked and they did sign it. > > In the future, please first ask a googler to verify that they signed the CLA :) Will do! :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
