Keyboard Shortcuts
File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(223)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 7911043: NOT FOR REVIEW - preliminary bg attachment fixed work

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 9 months ago by vollick
Modified:
12 years, 9 months ago
Reviewers:
danakj, hartmanng
Base URL:
git://git.webkit.org/WebKit.git@master
Visibility:
Public.
NOT FOR REVIEW - preliminary bg attachment fixed work BUG=

Patch Set 1 #

Patch Set 2 : . #

Total comments: 4

Patch Set 3 : . #

Total comments: 8

Patch Set 4 : . #

Patch Set 5 : . #

Created: 12 years, 9 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -21 lines) Patch
A + LayoutTests/platform/chromium/compositing/fixed-body-background-positioned.html View 1 2 chunks +6 lines, -3 lines 0 comments Download
A LayoutTests/platform/chromium/compositing/fixed-body-background-positioned-expected.png View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A + LayoutTests/platform/chromium/compositing/fixed-body-background-positioned-expected.txt View 1 1 chunk +0 lines, -7 lines 0 comments Download
M Source/WebCore/page/Settings.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/WebCore/page/Settings.in View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/WebCore/platform/graphics/GraphicsLayer.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp View 1 1 chunk +5 lines, -0 lines 0 comments Download
M Source/WebCore/rendering/RenderLayerBacking.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/WebCore/rendering/RenderLayerBacking.cpp View 1 2 3 4 7 chunks +12 lines, -8 lines 0 comments Download
M Source/WebCore/rendering/RenderLayerCompositor.cpp View 1 2 3 4 3 chunks +25 lines, -2 lines 0 comments Download
M Source/WebKit/chromium/public/WebSettings.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/WebKit/chromium/src/WebSettingsImpl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/WebKit/chromium/src/WebSettingsImpl.cpp View 1 1 chunk +6 lines, -2 lines 0 comments Download
M Tools/DumpRenderTree/chromium/DumpRenderTree.cpp View 1 4 chunks +5 lines, -0 lines 0 comments Download
M Tools/DumpRenderTree/chromium/TestRunner/public/WebPreferences.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M Tools/DumpRenderTree/chromium/TestRunner/src/WebPreferences.cpp View 1 2 chunks +2 lines, -0 lines 0 comments Download
M Tools/DumpRenderTree/chromium/TestShell.h View 1 2 chunks +2 lines, -0 lines 0 comments Download
M Tools/DumpRenderTree/chromium/TestShell.cpp View 1 2 chunks +2 lines, -0 lines 0 comments Download
Total messages: 5
|
hartmanng
Is the png really needed? Just asking because I make these by accident and don't ...
12 years, 9 months ago (2013年03月20日 20:54:27 UTC) #1 
Is the png really needed?
Just asking because I make these by accident and don't notice sometimes.
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
A comment would be good here, I think. In particular, in the else if, why do you
need to call that remove*() function if that wasn't necessary before this patch?
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: if (Settings* settings
= m_renderView->document()->settings())
could just be combined into one if
Sign in to reply to this message.
vollick
Yep, the png was on purpose. https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/RenderLayerBacking.cpp File Source/WebCore/rendering/RenderLayerBacking.cpp (right): https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/RenderLayerBacking.cpp#newcode871 Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM) On ...
12 years, 9 months ago (2013年03月20日 21:08:28 UTC) #2 
Yep, the png was on purpose.
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
On 2013年03月20日 20:54:27, hartmanng wrote:
> A comment would be good here, I think. In particular, in the else if, why do
you
> need to call that remove*() function if that wasn't necessary before this
patch?
Done.
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
https://codereview.appspot.com/7911043/diff/2001/Source/WebCore/rendering/Ren...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: if (Settings* settings
= m_renderView->document()->settings())
On 2013年03月20日 20:54:27, hartmanng wrote:
> could just be combined into one if
Done. Combined it into 0 ifs. :)
Sign in to reply to this message.
hartmanng
lgtm
12 years, 9 months ago (2013年03月20日 21:09:07 UTC) #3 
lgtm
Sign in to reply to this message.
danakj
yum, plumbing. i'm a noob so i have questions. https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/RenderLayerBacking.cpp File Source/WebCore/rendering/RenderLayerBacking.cpp (right): https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/RenderLayerBacking.cpp#newcode871 Source/WebCore/rendering/RenderLayerBacking.cpp:871: ...
12 years, 9 months ago (2013年03月20日 21:54:54 UTC) #4 
yum, plumbing. i'm a noob so i have questions.
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
how come mac doesn't need this?
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:882:
frameView->addViewportConstrainedObject(renderer());
is it ok to add the same renderer() more than once? would that happen?
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: return
renderViewBacking &&
Can you explain why false if !renderViewBacking, other than safari would be?
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2291:
renderView->document()->settings() &&
m_renderView?
Sign in to reply to this message.
vollick
Thanks for the review! https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/RenderLayerBacking.cpp File Source/WebCore/rendering/RenderLayerBacking.cpp (right): https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/RenderLayerBacking.cpp#newcode871 Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM) On 2013年03月20日 21:54:54, ...
12 years, 9 months ago (2013年03月21日 00:43:51 UTC) #5 
Thanks for the review!
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:871: #if PLATFORM(CHROMIUM)
On 2013年03月20日 21:54:54, danakj wrote:
> how come mac doesn't need this?
Good question. Mac does its registration in
RenderLayerCompositor::fixedRootBackgroundLayerChanged (which gets called from
the RenderLayerBacking when the background layer is created). That makes the
assumption that the background layer is only ever going to be used for fixed
root backgrounds (currently true). I've moved this code to the same spot for
symmetry.
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerBacking.cpp:882:
frameView->addViewportConstrainedObject(renderer());
On 2013年03月20日 21:54:54, danakj wrote:
> is it ok to add the same renderer() more than once? would that happen?
Yep, it's ok. It's a set.
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2290: return
renderViewBacking &&
On 2013年03月20日 21:54:54, danakj wrote:
> Can you explain why false if !renderViewBacking, other than safari would be?
You have to have a backing to have a composited fixed root bg layer -- it's what
manages its creation, insertion into the mini hierarchy, positioning, etc.
https://codereview.appspot.com/7911043/diff/15001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2291:
renderView->document()->settings() &&
On 2013年03月20日 21:54:54, danakj wrote:
> m_renderView?
This had me confused. I wondered "how could this ever compile? I must have the
#PLATFORM thing wrong." But no, it just didn't compile. Apparently I.. I
uploaded without even compiling. So much shame.
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /