Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1532473002:
Add a origin clean flag in ImageBitmap class (Closed)
|
Created:
5 years ago by xidachen Modified:
4 years, 11 months ago Reviewers:
Justin Novosad CC:
chromium-reviews, philipj_slow, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a origin clean flag in ImageBitmap class
This CL adds a origin clean flag, such that in the case when the source
of the ImageBitmap contains cross-origin content, we can simply set this
flag instead of reject the promise.
BUG=569779
Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630
Cr-Commit-Position: refs/heads/master@{#368595}
Committed: https://crrev.com/f3717a3a3a25aa5ddd9dfd4234f09ff13f001575
Cr-Commit-Position: refs/heads/master@{#368681}
Patch Set 1 #
Total comments: 13
Patch Set 2 : better origin clean flag #Patch Set 3 : handle canvas #
Total comments: 4
Patch Set 4 : better layout test #
Total comments: 12
Patch Set 5 : more on layout test #
Total comments: 4
Patch Set 6 : using promise chain in layout test #
Total comments: 10
Patch Set 7 : better layout test #Patch Set 8 : correct layout test #Patch Set 9 : change flag for image and staticbitmapimage #
Total comments: 1
Patch Set 10 : handle createImageBitmap(Blob) #Patch Set 11 : StaticBitmapImage now has a second parameter for origin clean flag #
Total comments: 8
Patch Set 12 : take care of single security origin #
Total comments: 2
Patch Set 13 : Roll back to PS#10 which works, and remove createImageBitmap(image) #
Total comments: 2
Patch Set 14 : use StaticBitmapImage's origin flag for ImageBitmap #Patch Set 15 : fix compilation error #Messages
Total messages: 50 (13 generated)
xidachen@chromium.org changed reviewers: + junov@chromium.org
PTAL
Where are the tests? See third_party/WebKit/LayoutTests/http/tests/security/canvas* for inspiration https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:73: ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas, const IntRect& cropRect) This constructor should transfer the origin clean flag of the canvas element. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: ImageBitmap::ImageBitmap(ImageBitmap* bitmap, const IntRect& cropRect) This constructor should take the value of the origin clean flag of 'bitmap' https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:106: ImageBitmap::ImageBitmap(Image* image, const IntRect& cropRect) this can be cross-origin. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:111: ImageBitmap::ImageBitmap(PassRefPtr<StaticBitmapImage> image) this can be cross-origin https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:116: PassRefPtr<StaticBitmapImage> ImageBitmap::transfer() the transfer logic needs some special precautions. If the ImageBitmap is being transferred to a same-origin scriptthe we need to transfer the value of the origin-clean flag. Otherwise, we need to set the value of the flag to true. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.h:31: static PassRefPtrWillBeRawPtr<ImageBitmap> create(HTMLImageElement*, const IntRect&, bool originCleanFlag = true); The value of the origin clean flag should not be an argument, it should be inferred from the HTMLImageElement. use HTMLImageElement::wouldTaintOrigin(). This will avoid duplicating the security logic. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.h:32: static PassRefPtrWillBeRawPtr<ImageBitmap> create(HTMLVideoElement*, const IntRect&, bool originCleanFlag = true); same here
https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:73: ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas, const IntRect& cropRect) On 2015/12/16 03:34:35, Justin Novosad wrote: > This constructor should transfer the origin clean flag of the canvas element. Done. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: ImageBitmap::ImageBitmap(ImageBitmap* bitmap, const IntRect& cropRect) On 2015/12/16 03:34:35, Justin Novosad wrote: > This constructor should take the value of the origin clean flag of 'bitmap' Done. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:106: ImageBitmap::ImageBitmap(Image* image, const IntRect& cropRect) On 2015/12/16 03:34:35, Justin Novosad wrote: > this can be cross-origin. Done. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:111: ImageBitmap::ImageBitmap(PassRefPtr<StaticBitmapImage> image) On 2015/12/16 03:34:35, Justin Novosad wrote: > this can be cross-origin Done. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.h:31: static PassRefPtrWillBeRawPtr<ImageBitmap> create(HTMLImageElement*, const IntRect&, bool originCleanFlag = true); On 2015/12/16 03:34:35, Justin Novosad wrote: > The value of the origin clean flag should not be an argument, it should be > inferred from the HTMLImageElement. use HTMLImageElement::wouldTaintOrigin(). > This will avoid duplicating the security logic. Done. https://codereview.chromium.org/1532473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.h:32: static PassRefPtrWillBeRawPtr<ImageBitmap> create(HTMLVideoElement*, const IntRect&, bool originCleanFlag = true); On 2015/12/16 03:34:35, Justin Novosad wrote: > same here Done.
https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:6: description("The image bitmap factories should not throw exceptions on cross-origin access."); The test needs to do more. You need to verify that the tainting worked. I would suggest writing a shouldBeTainted(imageBitmap) method that draws the imageBitmap into a fresh canvas, and then tries to call getImageData on the canvas, which should throw a SecurityError exception. Also, you need to cover the case where the image passed to createImageBitmap is a tainted ImageBitmap. https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:89: Document::create().get()); I think this should use imageElement->document(), same for all the changes below.
https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:6: description("The image bitmap factories should not throw exceptions on cross-origin access."); On 2015/12/23 18:06:34, Justin Novosad wrote: > The test needs to do more. You need to verify that the tainting worked. I would > suggest writing a shouldBeTainted(imageBitmap) method that draws the imageBitmap > into a fresh canvas, and then tries to call getImageData on the canvas, which > should throw a SecurityError exception. Also, you need to cover the case where > the image passed to createImageBitmap is a tainted ImageBitmap. Done. https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/1532473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:89: Document::create().get()); On 2015/12/23 18:06:34, Justin Novosad wrote: > I think this should use imageElement->document(), > same for all the changes below. Done.
https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:11: function shouldBeAccepted(promise, message) { -> shouldBeAcceptedAndTainted https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:23: function shouldBeTaint(imageBitmap) { -> shouldBeTainted https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:41: image.src = 'http://localhost:8080/security/resources/abe.png'; This image is being re-use repeatedly now. I would suggest simplifying the test by loading the image once into a global scope var and running the Promise chain from the image onload handler https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:49: image.addEventListener('load', resolve.bind(undefined, image)); This is not right. you are resolving the promise with the image element instead of the intended image bitmap. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:52: var imageBitmap = createImageBitmap(image); This is not correct 'imageBitmap' is a promise object, and it is just dropped on the floor here. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:59: image.addEventListener('load', resolve.bind(undefined, image)); This is not right. You want to resolve the promise with the canvas created below.
https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:11: function shouldBeAccepted(promise, message) { On 2015/12/29 17:33:50, Justin Novosad wrote: > -> shouldBeAcceptedAndTainted Done. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:23: function shouldBeTaint(imageBitmap) { On 2015/12/29 17:33:50, Justin Novosad wrote: > -> shouldBeTainted Done. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:41: image.src = 'http://localhost:8080/security/resources/abe.png'; On 2015/12/29 17:33:50, Justin Novosad wrote: > This image is being re-use repeatedly now. I would suggest simplifying the test > by loading the image once into a global scope var and running the Promise chain > from the image onload handler Done. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:49: image.addEventListener('load', resolve.bind(undefined, image)); On 2015/12/29 17:33:50, Justin Novosad wrote: > This is not right. you are resolving the promise with the image element instead > of the intended image bitmap. Done. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:52: var imageBitmap = createImageBitmap(image); On 2015/12/29 17:33:50, Justin Novosad wrote: > This is not correct 'imageBitmap' is a promise object, and it is just dropped on > the floor here. Done. https://codereview.chromium.org/1532473002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:59: image.addEventListener('load', resolve.bind(undefined, image)); On 2015/12/29 17:33:50, Justin Novosad wrote: > This is not right. You want to resolve the promise with the canvas created > below. Done.
https://codereview.chromium.org/1532473002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:50: context.drawImage(image, 0, 0, 10, 10); Add a comment that this is to taint the canvas https://codereview.chromium.org/1532473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:55: Promise.all([p1, p2, p3]).then(function() { Although this is clean and concise, it makes execution order non-deterministic, which is a problem in tests. We want the PASS/FAIL messages to be printed in a predictable order. I recommend using a chain (create p2 after p1 was resolved, etc.)
https://codereview.chromium.org/1532473002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:50: context.drawImage(image, 0, 0, 10, 10); On 2015/12/30 20:42:18, Justin Novosad wrote: > Add a comment that this is to taint the canvas Done. https://codereview.chromium.org/1532473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:55: Promise.all([p1, p2, p3]).then(function() { On 2015/12/30 20:42:18, Justin Novosad wrote: > Although this is clean and concise, it makes execution order non-deterministic, > which is a problem in tests. We want the PASS/FAIL messages to be printed in a > predictable order. I recommend using a chain (create p2 after p1 was resolved, > etc.) Done.
https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:31: testFailed("FAIL: ImageBitmap is not tainted."); No need for "FAIL:" because testFailed already adds that https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:33: testPassed("PASS: ImageBitmap is tainted. Threw error: " + e); No need for "PASS:". Notice the lines starting in "PASS PASS" in the test results. https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:56: then(function() { inconsistent line wrapping ("then" vs. ".then") https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:60: shouldBeAcceptedAndTainted(createImageBitmap(video, 0, 0, 10, 10), 'video').then(finishJSTest, finishJSTest); The rejection handlers should print a failure message. Otherwise, you could get a test that fails, yet only prints "PASS" messages, which is weird. https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:62: }, function() { "function() { myFunction(); }" => "myFunction"
https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html (right): https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:31: testFailed("FAIL: ImageBitmap is not tainted."); On 2016/01/04 20:45:51, Justin Novosad wrote: > No need for "FAIL:" because testFailed already adds that Acknowledged. https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:33: testPassed("PASS: ImageBitmap is tainted. Threw error: " + e); On 2016/01/04 20:45:51, Justin Novosad wrote: > No need for "PASS:". Notice the lines starting in "PASS PASS" in the test > results. Acknowledged. https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:56: then(function() { On 2016/01/04 20:45:51, Justin Novosad wrote: > inconsistent line wrapping ("then" vs. ".then") Acknowledged. https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:60: shouldBeAcceptedAndTainted(createImageBitmap(video, 0, 0, 10, 10), 'video').then(finishJSTest, finishJSTest); On 2016/01/04 20:45:51, Justin Novosad wrote: > The rejection handlers should print a failure message. Otherwise, you could get > a test that fails, yet only prints "PASS" messages, which is weird. Acknowledged. https://codereview.chromium.org/1532473002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/cross-origin-createImageBitmap.html:62: }, function() { On 2016/01/04 20:45:51, Justin Novosad wrote: > "function() { myFunction(); }" => "myFunction" Acknowledged.
lgtm
The CQ bit was checked by xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532473002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1532473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1532473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:115: m_isOriginClean = !image->currentFrameHasSingleSecurityOrigin(); Is this what we want? For StaticBitmapImage&Image, the currentFrameHasSingleSecurityOrigin() is set to be always false. This will set the origin clean flag of this ImageBitmap to be always clean.
Hi Justin, PS#10 is the one that works. PTAL.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532473002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532473002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL.
https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.h:77: bool m_isOriginClean = true; This is redundant now that we have a notion of origin clean in StaticBitmap Image. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:395: return StaticBitmapImage::create(firstFrame, true); Really? A bitmap Image can be cross-origin. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:163: return StaticBitmapImage::create(snapshot, true); Really? An image buffer may be associated with a canvas that is not origin clean. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp:77: RefPtr<Image> image = StaticBitmapImage::create(snapshot.release(), true); See Rule 10 of the Names section of the style guide (https://www.chromium.org/blink/coding-style#TOC-Names)
Hi Justin, I made some changes here. Please correct me if I am wrong: ImageBitmap has one thing only, which is StaticBitmapImage, so we should still use StaticBitmapImage's origin clean flag to tell whether ImageBitmap is origin clean or not. The only thing we need to be careful is at the call site of StaticBitmapImage constructor, where we need to think carefully whether it is origin clean or not. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.h:77: bool m_isOriginClean = true; On 2016/01/07 17:12:01, Justin Novosad wrote: > This is redundant now that we have a notion of origin clean in StaticBitmap > Image. Acknowledged. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:395: return StaticBitmapImage::create(firstFrame, true); On 2016/01/07 17:12:01, Justin Novosad wrote: > Really? A bitmap Image can be cross-origin. Acknowledged. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:163: return StaticBitmapImage::create(snapshot, true); On 2016/01/07 17:12:01, Justin Novosad wrote: > Really? An image buffer may be associated with a canvas that is not origin > clean. Acknowledged. https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp (right): https://codereview.chromium.org/1532473002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp:77: RefPtr<Image> image = StaticBitmapImage::create(snapshot.release(), true); On 2016/01/07 17:12:01, Justin Novosad wrote: > See Rule 10 of the Names section of the style guide > (https://www.chromium.org/blink/coding-style#TOC-Names) Acknowledged.
https://codereview.chromium.org/1532473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/GraphicsTypes.h (right): https://codereview.chromium.org/1532473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/GraphicsTypes.h:92: enum SingleSecurityOriginType { I would rename this SecurityOriginMode. With values SingleSecurityOrigin and NoSingleSecurityOrigin. Also, this definition should be moved to StaticBitmapImage.h. We only use this file for types that do not have a common header for all the places that use the type. https://codereview.chromium.org/1532473002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/1532473002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:31: bool m_isOriginClean; As discussed live, I would like this to be renamed to m_hasSingleSecurityOrigin for clarity. Also, you should declared it to use type SingleSecurityOriginType instead of bool.
On 2016/01/08 19:08:43, Justin Novosad wrote: > As discussed live, I would like this to be renamed to m_hasSingleSecurityOrigin > for clarity. Also, you should declared it to use type SingleSecurityOriginType > instead of bool. Actually, when we put both comments together, this should be SecurityOriginMode m_securityOriginMode
Hi Justin, PS#13 should work. PTAL.
https://codereview.chromium.org/1532473002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/1532473002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.h:77: bool m_isOriginClean = true; Can we safely get rid of this and just always use the origin clean flag in m_image? Having the flag in both places and possibly out of sync is a potential source of confusion.
https://codereview.chromium.org/1532473002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/1532473002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.h:77: bool m_isOriginClean = true; On 2016/01/08 22:09:39, Justin Novosad wrote: > Can we safely get rid of this and just always use the origin clean flag in > m_image? Having the flag in both places and possibly out of sync is a potential > source of confusion. This makes perfect sense and it is not hard. Since we have both getter and setter function for StaticBitmapImage's origin clean flag, we just call m_image->getOriginCleanflag or m_image->setOriginCleanflag to get it for ImageBitmap.
lgtm
The CQ bit was checked by junov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532473002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532473002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 ========== to ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1577783003/ by treib@chromium.org. The reason for reverting is: Seems to have broken Oilpan builds: https://build.chromium.org/p/chromium.webkit/buildstatus?builder=WebKit%20Mac... https://build.chromium.org/p/chromium.webkit/buildstatus?builder=WebKit%20Lin....
Message was sent while issue was closed.
It was a careless mistake that cause oilpan failure, I used enable_oilpan=true to compile this time, should be good to go.
Message was sent while issue was closed.
Description was changed from ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} ========== to ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} ==========
lgtm
The CQ bit was checked by xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532473002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532473002/280001
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532473002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532473002/280001
Message was sent while issue was closed.
Description was changed from ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} ========== to ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} ========== to ========== Add a origin clean flag in ImageBitmap class This CL adds a origin clean flag, such that in the case when the source of the ImageBitmap contains cross-origin content, we can simply set this flag instead of reject the promise. BUG=569779 Committed: https://crrev.com/10b9b4435e25fb8ede2122482426ae81c7980630 Cr-Commit-Position: refs/heads/master@{#368595} Committed: https://crrev.com/f3717a3a3a25aa5ddd9dfd4234f09ff13f001575 Cr-Commit-Position: refs/heads/master@{#368681} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f3717a3a3a25aa5ddd9dfd4234f09ff13f001575 Cr-Commit-Position: refs/heads/master@{#368681} |
