Description was changed from
==========
[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, images will now be displayed on a #0E0E0E
page background, while transparent images are displayed with a
checkerboard background.
Intent to implement:
https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81...
BUG=650455
==========
to
==========
[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-jkt81...
BUG=650455
==========
Overall looks great, just one small suggestion.
https://codereview.chromium.org/2445413003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:203: void
ImageDocument::finishedParsing() {
This isn't your fault but I think the call to finishedParsing in
ImageDocumentParser is a bug.
Document::finishedParsing kicks off the DOMContentLoaded event which is
generally when the document has finished loading but resources may still be
coming in. It makes sense for HTMLViewSourceParser to call Document::finish
because there are no subresources to load, but I don't think it's right for
ImageDocumentParser. I wonder if this is related to why we weren't getting the
onload event?
What do you think of the following tweak to this patch which avoids adding a
virtual to Document?
1) Add "void ImageDocument::imageLoaded()" which changes the background.
2) In the cachedImage() conditional in ImageDocumentParser::finish(), add a call
to document()->imageLoaded().
gone
UX has settled on the dynamic checker sizing thing, as far as I can tell. ...
UX has settled on the dynamic checker sizing thing, as far as I can tell. How's
this particular implementation looking?
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 ...
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:267: } else if
(m_shrinkToFitMode == Viewport) {
On 2016/10/28 21:20:48, pdr. wrote:
> Because many desktops have touchscreens these days, should we unconditionally
> add the touch event listeners?
The touch listeners applied only to viewport mode because of the difference in
zooming methods: desktop doesn't dynamically size the checkers because it can
render them all at the correct size whenever the image is fully zoomed in or
fully zoomed out.
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:381: // Unfortunately, the
page scale is not properly initialized when this
On 2016/10/28 21:20:48, pdr. wrote:
> This workaround is unfortunate. Can you think of any way to avoid it? For
> example, could we assume the initial scale is 1, or always use the width /
> divwidth codepath?
Yeah, I'm not happy with it either, which is why I was trying to push for not
dynamically changing the checker size :/
This is the only way I could think of to figure out what the initial scale
really needed to be... the scale is 1.0 means that the checkers will be exactly
10px * 10px, regardless of how wide the image actually is. The gigantic 4k X 4k
test image I had, for example, ends up with an initial scale of 0.1.
Oddly, the initial scale for all images on my phone was set to ~0.367 or
something strange when it really needed to be width/divwidth, and it's still not
set properly when the image finishes loading (hence the boolean). Always
setting the checker size to width/divwidth fails to account for the scale
changing, meaning that the checkers will be enlarged or shrunk with the exact
same scale as the image, and will never try to be a scale appropriate size :/
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 ...
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:267: } else if
(m_shrinkToFitMode == Viewport) {
On 2016/10/28 at 21:36:20, dfalcantara (slow 10.24) wrote:
> On 2016/10/28 21:20:48, pdr. wrote:
> > Because many desktops have touchscreens these days, should we
unconditionally
> > add the touch event listeners?
>
> The touch listeners applied only to viewport mode because of the difference in
zooming methods: desktop doesn't dynamically size the checkers because it can
render them all at the correct size whenever the image is fully zoomed in or
fully zoomed out.
ah, sgtm.
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:381: // Unfortunately, the
page scale is not properly initialized when this
On 2016/10/28 at 21:36:20, dfalcantara (slow 10.24) wrote:
> On 2016/10/28 21:20:48, pdr. wrote:
> > This workaround is unfortunate. Can you think of any way to avoid it? For
> > example, could we assume the initial scale is 1, or always use the width /
> > divwidth codepath?
>
> Yeah, I'm not happy with it either, which is why I was trying to push for not
dynamically changing the checker size :/
>
> This is the only way I could think of to figure out what the initial scale
really needed to be... the scale is 1.0 means that the checkers will be exactly
10px * 10px, regardless of how wide the image actually is. The gigantic 4k X 4k
test image I had, for example, ends up with an initial scale of 0.1.
>
> Oddly, the initial scale for all images on my phone was set to ~0.367 or
something strange when it really needed to be width/divwidth, and it's still not
set properly when the image finishes loading (hence the boolean). Always
setting the checker size to width/divwidth fails to account for the scale
changing, meaning that the checkers will be enlarged or shrunk with the exact
same scale as the image, and will never try to be a scale appropriate size :/
Any idea where 0.367 comes from? Could be worth figuring that out.
I'm not really sure where to start on this.
An idea: updateImageStyle is called from many places. Is the scale wrong on the
first call from all of them? For some background, documents have a "lifecycle"
state machine where we go through several states to render a page
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...).
It could be that the scale isn't available until one of those later lifecycle
states, and maybe the call from createDocumentStructure is just too early? You
could try testing if the scale is available when other callsites to
updateImageStyle.
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 ...
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:267: } else if
(m_shrinkToFitMode == Viewport) {
On 2016/10/28 at 21:36:20, dfalcantara (slow 10.24) wrote:
> On 2016/10/28 21:20:48, pdr. wrote:
> > Because many desktops have touchscreens these days, should we
unconditionally
> > add the touch event listeners?
>
> The touch listeners applied only to viewport mode because of the difference in
zooming methods: desktop doesn't dynamically size the checkers because it can
render them all at the correct size whenever the image is fully zoomed in or
fully zoomed out.
ah, sgtm.
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:381: // Unfortunately, the
page scale is not properly initialized when this
On 2016/10/28 at 21:36:20, dfalcantara (slow 10.24) wrote:
> On 2016/10/28 21:20:48, pdr. wrote:
> > This workaround is unfortunate. Can you think of any way to avoid it? For
> > example, could we assume the initial scale is 1, or always use the width /
> > divwidth codepath?
>
> Yeah, I'm not happy with it either, which is why I was trying to push for not
dynamically changing the checker size :/
>
> This is the only way I could think of to figure out what the initial scale
really needed to be... the scale is 1.0 means that the checkers will be exactly
10px * 10px, regardless of how wide the image actually is. The gigantic 4k X 4k
test image I had, for example, ends up with an initial scale of 0.1.
>
> Oddly, the initial scale for all images on my phone was set to ~0.367 or
something strange when it really needed to be width/divwidth, and it's still not
set properly when the image finishes loading (hence the boolean). Always
setting the checker size to width/divwidth fails to account for the scale
changing, meaning that the checkers will be enlarged or shrunk with the exact
same scale as the image, and will never try to be a scale appropriate size :/
Any idea where 0.367 comes from? Could be worth figuring that out.
I'm not really sure where to start on this.
An idea: updateImageStyle is called from many places. Is the scale wrong on the
first call from all of them? For some background, documents have a "lifecycle"
state machine where we go through several states to render a page
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...).
It could be that the scale isn't available until one of those later lifecycle
states, and maybe the call from createDocumentStructure is just too early? You
could try testing if the scale is available when other callsites to
updateImageStyle.
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 ...
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:381: // Unfortunately, the
page scale is not properly initialized when this
On 2016/10/28 22:16:50, pdr. wrote:
> On 2016/10/28 at 21:36:20, dfalcantara (slow 10.24) wrote:
> > On 2016/10/28 21:20:48, pdr. wrote:
> > > This workaround is unfortunate. Can you think of any way to avoid it? For
> > > example, could we assume the initial scale is 1, or always use the width /
> > > divwidth codepath?
> >
> > Yeah, I'm not happy with it either, which is why I was trying to push for
not
> dynamically changing the checker size :/
> >
> > This is the only way I could think of to figure out what the initial scale
> really needed to be... the scale is 1.0 means that the checkers will be
exactly
> 10px * 10px, regardless of how wide the image actually is. The gigantic 4k X
4k
> test image I had, for example, ends up with an initial scale of 0.1.
> >
> > Oddly, the initial scale for all images on my phone was set to ~0.367 or
> something strange when it really needed to be width/divwidth, and it's still
not
> set properly when the image finishes loading (hence the boolean). Always
> setting the checker size to width/divwidth fails to account for the scale
> changing, meaning that the checkers will be enlarged or shrunk with the exact
> same scale as the image, and will never try to be a scale appropriate size :/
>
> Any idea where 0.367 comes from? Could be worth figuring that out.
>
> I'm not really sure where to start on this.
>
> An idea: updateImageStyle is called from many places. Is the scale wrong on
the
> first call from all of them? For some background, documents have a "lifecycle"
> state machine where we go through several states to render a page
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...).
> It could be that the scale isn't available until one of those later lifecycle
> states, and maybe the call from createDocumentStructure is just too early? You
> could try testing if the scale is available when other callsites to
> updateImageStyle.
Did some more experimentation about found that the page scale factor seems to be
set correctly after the call to document()->finishedParsing() — which also
unfortunately detaches the parser from the document() and prevents us from
calling document()->imageLoaded() :/
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
WDYT about doing something like this, where a task is posted when the image
finishes loading?
We can't override detach() because the page scale hasn't been updated at that
point, and I don't think it's safe to cache the Document* and then use it after
detach() has been called in finishedParsing().
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
Sorry about the slow review :/
https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:363:
postTask(BLINK_FROM_HERE, createSameThreadTask(&updateDocumentImageStyle),
This seems reasonable to me if we don't have another way (I agree with your
comment about document not being safe to touch).
We use width=device-width and explicitly set the image size... why can't we
compute the initial scale ourselves before it goes through to the visual
viewport? I played around with various image sizes and window sizes and I think
it's just std::min(viewportWidth, viewportHeight) / std::min(divWidth,
divHeight). This seemed to work in a few simple tests.
https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:368: StringBuilder
imageStyle;
Because this is called on every touch end/cancel event, WDYT of a TODO (for
future folks, not necessarily you) to avoid updates when the resulting style
doesn't change?
Something like:
// TODO(dfalcantara): Avoid no-op style changes to avoid unnecessary style churn
and repaints.
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
No worries on review speed; the parent task of adding a download manager to
chrome has been a never-ending font of tasks :)
I've also found another way we're getting bitten by being unable to monitor page
scale changes: double tapping to zoom in or out isn't captured by the touch
gestures. I tried catching double clicks but those didn't register (probably
because they're consumed before the event listener is fired). I don't think
it's a _huge_ deal, but it's something to be aware of.
In amusing news, the original first half of the change is being discussed on
reddit:
http://www.androidpolice.com/2016/11/01/chrome-now-displays-images-centered-s...https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:363:
postTask(BLINK_FROM_HERE, createSameThreadTask(&updateDocumentImageStyle),
On 2016/11/01 06:54:56, pdr. wrote:
> This seems reasonable to me if we don't have another way (I agree with your
> comment about document not being safe to touch).
>
> We use width=device-width and explicitly set the image size... why can't we
> compute the initial scale ourselves before it goes through to the visual
> viewport? I played around with various image sizes and window sizes and I
think
> it's just std::min(viewportWidth, viewportHeight) / std::min(divWidth,
> divHeight). This seemed to work in a few simple tests.
Got rid of the task and tried looking at more flags when this function is
called. Essentially I initialize the checker size when the image finishes
loading, but don't dynamically change it until the page has finished parsing.
Been testing this patch using the images on
https://inadequatelycaffeinated.appspot.com/clank/download_tests/download_tes...
They all seem to be fine if i just work with the divWidth; I'm hesitant to
use the divHeight because viewport height doesn't account for really tall
images.
https://codereview.chromium.org/2445413003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:368: StringBuilder
imageStyle;
On 2016/11/01 06:54:56, pdr. wrote:
> Because this is called on every touch end/cancel event, WDYT of a TODO (for
> future folks, not necessarily you) to avoid updates when the resulting style
> doesn't change?
>
> Something like:
> // TODO(dfalcantara): Avoid no-op style changes to avoid unnecessary style
churn
> and repaints.
WDYT about this version, where I compare the two style strings?
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
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
Running through the layout bots...
https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):
https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:61: int gBaseCheckerSize =
10;
On 2016/11/02 22:54:49, pdr. wrote:
> Nit: since this is a constant, wdyt of:
> const int kBaseCheckerSize = 10;
Ah forgot what the proper syntax was here. Done.
https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:400: m_checkerSize =
static_cast<int>(round(newCheckerSize));
On 2016/11/02 22:54:49, pdr. wrote:
> Nit: just "m_checkerSize = round(newCheckerSize);" ?
Done.
https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:426: if
(m_imageElement->getAttribute(styleAttr) != imageStyle.toAtomicString())
On 2016/11/02 22:54:49, pdr. wrote:
> This large string comparison is kinda unfortunate. WDYT about storing off the
> values instead, or just leaving this as a TODO for now?
After figuring out how to avoid the string comparison, and some slight
rearranging to move the body of imageLoaded() as an else clause to the
hasFinishedParsing() conditional above, I've ended up with something similar to
PS7 lol... I guess we at least understand what's going on a lot better, and it
avoids over-updating?
https://codereview.chromium.org/2445413003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:512: float
viewportAspectRatio = (float)viewportWidth / viewportHeight;
On 2016/11/02 22:54:49, pdr. wrote:
> Super nit: float viewportAspectRatio =
> frame()->host()->visualViewport().size().aspectRatio();
Done.
gone
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
4 years, 1 month ago
(2016-11-03 23:11:08 UTC)
#15
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/174000)
4 years, 1 month ago
(2016-11-04 00:33:49 UTC)
#19
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
Description was changed from ========== [Blink] Display images on a dark background Images displayed in ...
4 years, 1 month ago
(2016-11-04 06:57:40 UTC)
#28
Message was sent while issue was closed.
Description was changed from
==========
[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-jkt81...
BUG=650455
==========
to
==========
[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-jkt81...
BUG=650455
==========
commit-bot: I haz the power
Committed patchset #15 (id:280001)
4 years, 1 month ago
(2016-11-04 06:57:41 UTC)
#29
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
commit-bot: I haz the power
Description was changed from ========== [Blink] Display images on a dark background Images displayed in ...
4 years, 1 month ago
(2016-11-04 07:01:34 UTC)
#30
Message was sent while issue was closed.
Description was changed from
==========
[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-jkt81...
BUG=650455
==========
to
==========
[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-jkt81...
BUG=650455
Committed: https://crrev.com/98b66eba2a1509a2c7d2d6a4ecab6c270a761092
Cr-Commit-Position: refs/heads/master@{#429820}
==========
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/98b66eba2a1509a2c7d2d6a4ecab6c270a761092 Cr-Commit-Position: refs/heads/master@{#429820}
4 years, 1 month ago
(2016-11-04 07:01:38 UTC)
#31
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.
Base URL:
Comments: 21