Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(364)

Issue 2445413003: [Blink] Display images on a dark background (Closed)

Created:
4 years, 1 month ago by gone
Modified:
4 years, 1 month ago
Reviewers:
pdr.
CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Blink] Display images on a dark background Images displayed in a standalone main frame are now displayed on a color that is almost, but not quite, entirely unlike black. More specifically, all images will now be displayed on a #0E0E0E page background, while transparent images are displayed on a checkerboard image background. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81Ng/edit?usp=sharing BUG=650455 Committed: https://crrev.com/98b66eba2a1509a2c7d2d6a4ecab6c270a761092 Cr-Commit-Position: refs/heads/master@{#429820}

Patch Set 1 #

Patch Set 2 : Use finish #

Total comments: 1

Patch Set 3 : Comments, dynamic checkerboarding #

Patch Set 4 : Dynamically change checker size #

Patch Set 5 : Fixed cast #

Patch Set 6 : Rebasing #

Patch Set 7 : Rebased #

Total comments: 7

Patch Set 8 : Post task #

Patch Set 9 : Rebasing #

Total comments: 4

Patch Set 10 : Getting rid of the post #

Patch Set 11 : Rebasing #

Total comments: 9

Patch Set 12 : Comments #

Patch Set 13 : Rebase #

Patch Set 14 : Rebaselining #

Patch Set 15 : Rebase #

Messages

Total messages: 31 (12 generated)
pdr.
Overall looks great, just one small suggestion. https://codereview.chromium.org/2445413003/diff/20001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/20001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode203 third_party/WebKit/Source/core/html/ImageDocument.cpp:203: void ImageDocument::finishedParsing() ...
4 years, 1 month ago (2016-10-26 03:59:06 UTC) #3
gone
UX has settled on the dynamic checker sizing thing, as far as I can tell. ...
4 years, 1 month ago (2016-10-27 21:13:53 UTC) #4
pdr.
I'd like to avoid that workaround if at all possible. Otherwise things look good. https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp ...
4 years, 1 month ago (2016-10-28 21:20:48 UTC) #5
gone
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode267 third_party/WebKit/Source/core/html/ImageDocument.cpp:267: } else if (m_shrinkToFitMode == Viewport) { On 2016/10/28 ...
4 years, 1 month ago (2016-10-28 21:36:20 UTC) #6
pdr.
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode267 third_party/WebKit/Source/core/html/ImageDocument.cpp:267: } else if (m_shrinkToFitMode == Viewport) { On 2016/10/28 ...
4 years, 1 month ago (2016-10-28 22:16:50 UTC) #7
pdr.
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode267 third_party/WebKit/Source/core/html/ImageDocument.cpp:267: } else if (m_shrinkToFitMode == Viewport) { On 2016/10/28 ...
4 years, 1 month ago (2016-10-28 22:16:50 UTC) #8
gone
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode381 third_party/WebKit/Source/core/html/ImageDocument.cpp:381: // Unfortunately, the page scale is not properly initialized ...
4 years, 1 month ago (2016-10-28 23:42:22 UTC) #9
gone
WDYT about doing something like this, where a task is posted when the image finishes ...
4 years, 1 month ago (2016-10-31 20:04:02 UTC) #10
pdr.
Sorry about the slow review :/ https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode363 third_party/WebKit/Source/core/html/ImageDocument.cpp:363: postTask(BLINK_FROM_HERE, createSameThreadTask(&updateDocumentImageStyle), This ...
4 years, 1 month ago (2016-11-01 06:54:56 UTC) #11
gone
No worries on review speed; the parent task of adding a download manager to chrome ...
4 years, 1 month ago (2016-11-02 18:42:20 UTC) #12
pdr.
This is starting to look landable. Lets call the tower, reduce power, adjust layout tests, ...
4 years, 1 month ago (2016-11-02 22:54:50 UTC) #13
gone
Running through the layout bots... https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Source/core/html/ImageDocument.cpp File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Source/core/html/ImageDocument.cpp#newcode61 third_party/WebKit/Source/core/html/ImageDocument.cpp:61: int gBaseCheckerSize = 10; ...
4 years, 1 month ago (2016-11-03 01:28:41 UTC) #14
pdr.
LGTM
4 years, 1 month ago (2016-11-04 00:04:57 UTC) #17
gone
Thanks! Rolling the dice on the CQ...
4 years, 1 month ago (2016-11-04 01:07:32 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2445413003/280001
4 years, 1 month ago (2016-11-04 01:08:39 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/310884)
4 years, 1 month ago (2016-11-04 06:05:51 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2445413003/280001
4 years, 1 month ago (2016-11-04 06:11:09 UTC) #27
commit-bot: I haz the power
Committed patchset #15 (id:280001)
4 years, 1 month ago (2016-11-04 06:57:41 UTC) #29
commit-bot: I haz the power
4 years, 1 month ago (2016-11-04 07:01:38 UTC) #31
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/98b66eba2a1509a2c7d2d6a4ecab6c270a761092
Cr-Commit-Position: refs/heads/master@{#429820}

Powered by Google App Engine
This is Rietveld 408576698