Bug 85229 – Move RenderTableCell's row index to RenderTableRow

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Apart from this being cleaner from a concept perspective (the row index is a RenderTableRow concept), it also avoids having to keep track and update all the cells' indexes.

On top of that, it should enable more optimizations as the row now has an efficient way to query its row index.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 139525 [details]
Proposed refactoring 1: move the code to RenderTableRow and rename row() to rowIndex().

[Eric Seidel (no email)]

Comment on attachment 139525 [details]
Proposed refactoring 1: move the code to RenderTableRow and rename row() to rowIndex().

View in context: https://bugs.webkit.org/attachment.cgi?id=139525&action=review

> Source/WebCore/rendering/RenderTableRow.h:57
> +        if (UNLIKELY(rowIndex > maxRowIndex))
> +            CRASH();

Odd.  I wonder if this overflow can get us in trouble, since we don't actually crash on release systems...

[Ojan Vafai]

Comment on attachment 139525 [details]
Proposed refactoring 1: move the code to RenderTableRow and rename row() to rowIndex().

View in context: https://bugs.webkit.org/attachment.cgi?id=139525&action=review

Certainly looks like a big improvement. Please address comments before committing.

> Source/WebCore/rendering/RenderTableCell.cpp:333
> +    if (parent() && section() && oldStyle && style()->height() != oldStyle->height())

Shouldn't you continue to check rowWasSet here? Seems like the same applies. rowIndex could be unsetRowIndex, no? Actually, now that I look at RenderTableSection::addChild, it looks like it might not be possible. If you think it's not possible, maybe add an ASSERT?

> Source/WebCore/rendering/RenderTableCell.h:185
>      unsigned m_column : 31;

If you s/31/30/ here you should be able to get better big packing here.

> Source/WebCore/rendering/RenderTableRow.h:93
> +    unsigned m_rowIndex : 31;

Don't think we need to make this a bitfield now that it doesn't save us any memory.

[Abhishek Arya]

(In reply to comment #2)
> (From update of attachment 139525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139525&action=review
> 
> > Source/WebCore/rendering/RenderTableRow.h:57
> > +        if (UNLIKELY(rowIndex > maxRowIndex))
> > +            CRASH();
> 
> Odd.  I wonder if this overflow can get us in trouble, since we don't actually crash on release systems...

We do actually crash on release systems. e.g. testcase from ClusterFuzz. 

Testcase:
<table><tr style='-webkit-border-image: none; content: counter(ident46); '>><style>div {
</style><th colspan=2147483647><th>>

+----------------------------------------Release Build Stacktrace----------------------------------------+

/mnt/scratch0/clusterfuzz/slave-bot/builds/symbolized/release/asan-symbolized-linux-release-134098/chrome --no-first-run --single-process --disable-gpu-plugin --disable-gpu-rendering --disable-accelerated-compositing --disable-webgl --disable-accelerated-2d-canvas --user-data-dir=/mnt/scratch0/tmp/user_profile_chrome_0

LaunchProcess: failed to execvp:
/mnt/scratch0/clusterfuzz/slave-bot/builds/symbolized/release/asan-symbolized-linux-release-134098/nacl_helper_bootstrap
[11617:11617:29268359655:ERROR:nacl_fork_delegate_linux.cc(106)] Bad NaCl helper startup ack (0 bytes)
[11615:11627:29268402664:ERROR:proxy_service_factory.cc(84)] Cannot use V8 Proxy resolver in single process mode.
[11615:11627:29268640692:ERROR:proxy_service_factory.cc(84)] Cannot use V8 Proxy resolver in single process mode.
1   0x7fd08acb73da
2   0x7fd08acb7015
3   0x7fd08acb4cd1
4   0x7fd089b7ac1f
5   0x7fd089b59706
6   0x7fd089b2d24e
7   0x7fd08a05f009
8   0x7fd08a05ed63
9   0x7fd089ff8cc5
10  0x7fd089fd80e5
11  0x7fd089fd933a
12  0x7fd08cd27b1e
13  0x7fd08a6e343e
14  0x7fd089a2f5fc
15  0x7fd08a6e333b
16  0x7fd08a7406c2
17  0x7fd08a72c5cc
18  0x7fd08a741451
19  0x7fd089577582
20  0x7fd089578ed6
21  0x7fd089575a1e
22  0x7fd089574de7
23  0x7fd08947c642
24  0x7fd0887e75c4
25  0x7fd0887ed698
26  0x7fd0886f7623
27  0x7fd0886f7d89
28  0x7fd0886f80a2
29  0x7fd088704488
30  0x7fd0886f6e1c
31  0x7fd0886f5b18
ASAN:SIGSEGV
==11615== ERROR: AddressSanitizer crashed on unknown address 0x0000bbadbeef (pc 0x7fd08acb73f5 sp 0x7fd06eb65e80 bp 0x7fd06eb65e90 T16)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7fd08acb73f5 in WebCore::RenderTableCell::setCol(unsigned int) third_party/WebKit/Source/WebCore/rendering/RenderTableCell.h:57
    #1 0x7fd08acb7015 in WebCore::RenderTableSection::addCell(WebCore::RenderTableCell*, WebCore::RenderTableRow*) third_party/WebKit/Source/WebCore/rendering/RenderTableSection.cpp:271
    #2 0x7fd08acb4cd1 in WebCore::RenderTableRow::addChild(WebCore::RenderObject*, WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderTableRow.cpp:132
    #3 0x7fd089b7ac1f in WebCore::NodeRendererFactory::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:405
    #4 0x7fd089b59706 in WebCore::Node::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/Node.cpp:1427
    #5 0x7fd089b2d24e in WebCore::Element::attach() third_party/WebKit/Source/WebCore/dom/Element.cpp:960
    #6 0x7fd08a05f009 in WebCore::executeTask(WebCore::HTMLConstructionSiteTask&) third_party/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:101
    #7 0x7fd08a05ed63 in WebCore::HTMLConstructionSite::executeQueuedTasks() third_party/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:139
    #8 0x7fd089ff8cc5 in WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:461
    #9 0x7fd089fd80e5 in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:263
    #10 0x7fd089fd933a in WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:372
    #11 0x7fd08cd27b1e in WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long) third_party/WebKit/Source/WebCore/dom/DecodedDataDocumentParser.cpp:50
    #12 0x7fd08a6e343e in ~RefPtr third_party/WebKit/Source/WTF/wtf/RefPtr.h:56
    #13 0x7fd089a2f5fc in WebKit::FrameLoaderClientImpl::committedLoad(WebCore::DocumentLoader*, char const*, int) third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1129
    #14 0x7fd08a6e333b in void WTF::derefIfNotNull<WebCore::DocumentLoader>(WebCore::DocumentLoader*) third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52
    #15 0x7fd08a7406c2 in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:276
    #16 0x7fd08a72c5cc in void WTF::derefIfNotNull<WebCore::MainResourceLoader>(WebCore::MainResourceLoader*) third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52
    #17 0x7fd08a741451 in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:430
    #18 0x7fd089577582 in ResourceDispatcher::OnReceivedData(IPC::Message const&, int, base::FileDescriptor, int, int) content/common/resource_dispatcher.cc:404
    #19 0x7fd089578ed6 in bool ResourceMsg_DataReceived::Dispatch<ResourceDispatcher, ResourceDispatcher, int, base::FileDescriptor, int, int>(IPC::Message const*, ResourceDispatcher*, ResourceDispatcher*, void (ResourceDispatcher::*)(IPC::Message const&, int, base::FileDescriptor, int, int)) ./content/common/resource_messages.h:155
    #20 0x7fd089575a1e in ResourceDispatcher::DispatchMessage(IPC::Message const&) content/common/resource_dispatcher.cc:557
    #21 0x7fd089574de7 in ResourceDispatcher::OnMessageReceived(IPC::Message const&) content/common/resource_dispatcher.cc:326
    #22 0x7fd08947c642 in ChildThread::OnMessageReceived(IPC::Message const&) content/common/child_thread.cc:177
    #23 0x7fd0887e75c4 in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) ipc/ipc_channel_proxy.cc:269
    #24 0x7fd0887ed698 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void ()(IPC::ChannelProxy::Context* const&, IPC::Message const&)>::MakeItSo(base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, IPC::ChannelProxy::Context* const&, IPC::Message const&) ./base/bind_internal.h:897
    #25 0x7fd0886f7623 in MessageLoop::RunTask(base::PendingTask const&) base/message_loop.cc:459
    #26 0x7fd0886f7d89 in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop.cc:470
    #27 0x7fd0886f80a2 in MessageLoop::DoWork() base/message_loop.cc:647
    #28 0x7fd088704488 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_pump_default.cc:28
    #29 0x7fd0886f6e1c in MessageLoop::RunInternal() base/message_loop.cc:418
    #30 0x7fd0886f5b18 in MessageLoop::Run() base/message_loop.cc:301
    #31 0x7fd08876c461 in base::Thread::ThreadMain() base/threading/thread.cc:164
    #32 0x7fd0887614bc in base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:65
    #33 0x7fd08d53956c in __asan::AsanThread::ThreadStart() 
Stats: 30M malloced (51M for red zones) by 173565 calls
Stats: 1M realloced by 4424 calls
Stats: 23M freed by 127118 calls
Stats: 0M really freed by 0 calls
Stats: 116M (29708 full pages) mmaped in 29 calls
  mmaps   by size class: 8:163830; 9:16382; 10:12285; 11:4094; 12:2048; 13:512; 14:512; 15:128; 16:192; 17:32; 18:16; 20:4;
  mallocs by size class: 8:153839; 9:8310; 10:6959; 11:2351; 12:1276; 13:286; 14:318; 15:50; 16:151; 17:12; 18:12; 20:1;
  frees   by size class: 8:111742; 9:5707; 10:6082; 11:1922; 12:1037; 13:165; 14:290; 15:32; 16:129; 17:1; 18:10; 20:1;
  rfrees  by size class:
Stats: malloc large: 25 small slow: 676

[Julien Chaffraix]

Comment on attachment 139525 [details]
Proposed refactoring 1: move the code to RenderTableRow and rename row() to rowIndex().

View in context: https://bugs.webkit.org/attachment.cgi?id=139525&action=review

>> Source/WebCore/rendering/RenderTableCell.cpp:333
>> +    if (parent() && section() && oldStyle && style()->height() != oldStyle->height())
> 
> Shouldn't you continue to check rowWasSet here? Seems like the same applies. rowIndex could be unsetRowIndex, no? Actually, now that I look at RenderTableSection::addChild, it looks like it might not be possible. If you think it's not possible, maybe add an ASSERT?

The tests that prompted this check are passing even after that. The logic shouldn't allow this case anymore as we always set the row straight away (the logic on the cell side was open to race conditions as it didn't do it straight away). I will add the following ASSERT:

ASSERT(!row() || row()->rowIndexWasSet());

Adding back the rowIndexWasSet method.

>> Source/WebCore/rendering/RenderTableCell.h:185
>>      unsigned m_column : 31;
> 
> If you s/31/30/ here you should be able to get better big packing here.

You are totally right. However the change is big as-is so I didn't want to pollute it with an orthogonal - yet neat change. That will be moved to a follow-up bug.

>>> Source/WebCore/rendering/RenderTableRow.h:57
>>> +            CRASH();
>> 
>> Odd.  I wonder if this overflow can get us in trouble, since we don't actually crash on release systems...
> 
> We do actually crash on release systems. e.g. testcase from ClusterFuzz. 
> 
> Testcase:
> <table><tr style='-webkit-border-image: none; content: counter(ident46); '>><style>div {
> </style><th colspan=2147483647><th>>
> 
> +----------------------------------------Release Build Stacktrace----------------------------------------+
> 
> /mnt/scratch0/clusterfuzz/slave-bot/builds/symbolized/release/asan-symbolized-linux-release-134098/chrome --no-first-run --single-process --disable-gpu-plugin --disable-gpu-rendering --disable-accelerated-compositing --disable-webgl --disable-accelerated-2d-canvas --user-data-dir=/mnt/scratch0/tmp/user_profile_chrome_0
> 
> LaunchProcess: failed to execvp:
> /mnt/scratch0/clusterfuzz/slave-bot/builds/symbolized/release/asan-symbolized-linux-release-134098/nacl_helper_bootstrap
> [11617:11617:29268359655:ERROR:nacl_fork_delegate_linux.cc(106)] Bad NaCl helper startup ack (0 bytes)
> [11615:11627:29268402664:ERROR:proxy_service_factory.cc(84)] Cannot use V8 Proxy resolver in single process mode.
> [11615:11627:29268640692:ERROR:proxy_service_factory.cc(84)] Cannot use V8 Proxy resolver in single process mode.
> 1   0x7fd08acb73da
> 2   0x7fd08acb7015
> 3   0x7fd08acb4cd1
> 4   0x7fd089b7ac1f
> 5   0x7fd089b59706
> 6   0x7fd089b2d24e
> 7   0x7fd08a05f009
> 8   0x7fd08a05ed63
> 9   0x7fd089ff8cc5
> 10  0x7fd089fd80e5
> 11  0x7fd089fd933a
> 12  0x7fd08cd27b1e
> 13  0x7fd08a6e343e
> 14  0x7fd089a2f5fc
> 15  0x7fd08a6e333b
> 16  0x7fd08a7406c2
> 17  0x7fd08a72c5cc
> 18  0x7fd08a741451
> 19  0x7fd089577582
> 20  0x7fd089578ed6
> 21  0x7fd089575a1e
> 22  0x7fd089574de7
> 23  0x7fd08947c642
> 24  0x7fd0887e75c4
> 25  0x7fd0887ed698
> 26  0x7fd0886f7623
> 27  0x7fd0886f7d89
> 28  0x7fd0886f80a2
> 29  0x7fd088704488
> 30  0x7fd0886f6e1c
> 31  0x7fd0886f5b18
> ASAN:SIGSEGV
> ==11615== ERROR: AddressSanitizer crashed on unknown address 0x0000bbadbeef (pc 0x7fd08acb73f5 sp 0x7fd06eb65e80 bp 0x7fd06eb65e90 T16)
> AddressSanitizer can not provide additional info. ABORTING
>     #0 0x7fd08acb73f5 in WebCore::RenderTableCell::setCol(unsigned int) third_party/WebKit/Source/WebCore/rendering/RenderTableCell.h:57
>     #1 0x7fd08acb7015 in WebCore::RenderTableSection::addCell(WebCore::RenderTableCell*, WebCore::RenderTableRow*) third_party/WebKit/Source/WebCore/rendering/RenderTableSection.cpp:271
>     #2 0x7fd08acb4cd1 in WebCore::RenderTableRow::addChild(WebCore::RenderObject*, WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderTableRow.cpp:132
>     #3 0x7fd089b7ac1f in WebCore::NodeRendererFactory::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:405
>     #4 0x7fd089b59706 in WebCore::Node::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/Node.cpp:1427
>     #5 0x7fd089b2d24e in WebCore::Element::attach() third_party/WebKit/Source/WebCore/dom/Element.cpp:960
>     #6 0x7fd08a05f009 in WebCore::executeTask(WebCore::HTMLConstructionSiteTask&) third_party/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:101
>     #7 0x7fd08a05ed63 in WebCore::HTMLConstructionSite::executeQueuedTasks() third_party/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:139
>     #8 0x7fd089ff8cc5 in WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:461
>     #9 0x7fd089fd80e5 in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:263
>     #10 0x7fd089fd933a in WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:372
>     #11 0x7fd08cd27b1e in WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long) third_party/WebKit/Source/WebCore/dom/DecodedDataDocumentParser.cpp:50
>     #12 0x7fd08a6e343e in ~RefPtr third_party/WebKit/Source/WTF/wtf/RefPtr.h:56
>     #13 0x7fd089a2f5fc in WebKit::FrameLoaderClientImpl::committedLoad(WebCore::DocumentLoader*, char const*, int) third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1129
>     #14 0x7fd08a6e333b in void WTF::derefIfNotNull<WebCore::DocumentLoader>(WebCore::DocumentLoader*) third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52
>     #15 0x7fd08a7406c2 in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:276
>     #16 0x7fd08a72c5cc in void WTF::derefIfNotNull<WebCore::MainResourceLoader>(WebCore::MainResourceLoader*) third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52
>     #17 0x7fd08a741451 in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:430
>     #18 0x7fd089577582 in ResourceDispatcher::OnReceivedData(IPC::Message const&, int, base::FileDescriptor, int, int) content/common/resource_dispatcher.cc:404
>     #19 0x7fd089578ed6 in bool ResourceMsg_DataReceived::Dispatch<ResourceDispatcher, ResourceDispatcher, int, base::FileDescriptor, int, int>(IPC::Message const*, ResourceDispatcher*, ResourceDispatcher*, void (ResourceDispatcher::*)(IPC::Message const&, int, base::FileDescriptor, int, int)) ./content/common/resource_messages.h:155
>     #20 0x7fd089575a1e in ResourceDispatcher::DispatchMessage(IPC::Message const&) content/common/resource_dispatcher.cc:557
>     #21 0x7fd089574de7 in ResourceDispatcher::OnMessageReceived(IPC::Message const&) content/common/resource_dispatcher.cc:326
>     #22 0x7fd08947c642 in ChildThread::OnMessageReceived(IPC::Message const&) content/common/child_thread.cc:177
>     #23 0x7fd0887e75c4 in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) ipc/ipc_channel_proxy.cc:269
>     #24 0x7fd0887ed698 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void ()(IPC::ChannelProxy::Context* const&, IPC::Message const&)>::MakeItSo(base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, IPC::ChannelProxy::Context* const&, IPC::Message const&) ./base/bind_internal.h:897
>     #25 0x7fd0886f7623 in MessageLoop::RunTask(base::PendingTask const&) base/message_loop.cc:459
>     #26 0x7fd0886f7d89 in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop.cc:470
>     #27 0x7fd0886f80a2 in MessageLoop::DoWork() base/message_loop.cc:647
>     #28 0x7fd088704488 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_pump_default.cc:28
>     #29 0x7fd0886f6e1c in MessageLoop::RunInternal() base/message_loop.cc:418
>     #30 0x7fd0886f5b18 in MessageLoop::Run() base/message_loop.cc:301
>     #31 0x7fd08876c461 in base::Thread::ThreadMain() base/threading/thread.cc:164
>     #32 0x7fd0887614bc in base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:65
>     #33 0x7fd08d53956c in __asan::AsanThread::ThreadStart() 
> Stats: 30M malloced (51M for red zones) by 173565 calls
> Stats: 1M realloced by 4424 calls
> Stats: 23M freed by 127118 calls
> Stats: 0M really freed by 0 calls
> Stats: 116M (29708 full pages) mmaped in 29 calls
>   mmaps   by size class: 8:163830; 9:16382; 10:12285; 11:4094; 12:2048; 13:512; 14:512; 15:128; 16:192; 17:32; 18:16; 20:4;
>   mallocs by size class: 8:153839; 9:8310; 10:6959; 11:2351; 12:1276; 13:286; 14:318; 15:50; 16:151; 17:12; 18:12; 20:1;
>   frees   by size class: 8:111742; 9:5707; 10:6082; 11:1922; 12:1037; 13:165; 14:290; 15:32; 16:129; 17:1; 18:10; 20:1;
>   rfrees  by size class:
> Stats: malloc large: 25 small slow: 676

Violent comment!

Another proof are the ASSERT_DEATHs below that check that we do crash in this case. They are also running in Release (and I hope passing :)).

>> Source/WebCore/rendering/RenderTableRow.h:93
>> +    unsigned m_rowIndex : 31;
> 
> Don't think we need to make this a bitfield now that it doesn't save us any memory.

You are right again but I would rather keep it for 2 reasons: symmetry with the column case and also as it keeps one bit for future optimization.

[Julien Chaffraix]

Created attachment 139559 [details]
Patch for landing

[Ojan Vafai]

(In reply to comment #5)
> (From update of attachment 139525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139525&action=review
> 
> >> Source/WebCore/rendering/RenderTableCell.h:185
> >>      unsigned m_column : 31;
> > 
> > If you s/31/30/ here you should be able to get better big packing here.
> 
> You are totally right. However the change is big as-is so I didn't want to pollute it with an orthogonal - yet neat change. That will be moved to a follow-up bug.

A FIXME would be good.

> >> Source/WebCore/rendering/RenderTableRow.h:93
> >> +    unsigned m_rowIndex : 31;
> > 
> > Don't think we need to make this a bitfield now that it doesn't save us any memory.
> 
> You are right again but I would rather keep it for 2 reasons: symmetry with the column case and also as it keeps one bit for future optimization.

Meh. Up to you, but I don't think it makes sense to clutter header files with bitfields that aren't accomplishing anything. Symmetry with the column case doesn't really buy us anything since we clamp the value anyways and it doesn't let us share more code or anything.

[WebKit Review Bot]

Comment on attachment 139559 [details]
Patch for landing

Clearing flags on attachment: 139559

Committed r115705: <http://trac.webkit.org/changeset/115705>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

Comment on attachment 139525 [details]
Proposed refactoring 1: move the code to RenderTableRow and rename row() to rowIndex().

View in context: https://bugs.webkit.org/attachment.cgi?id=139525&action=review

>>>> Source/WebCore/rendering/RenderTableCell.h:185
>>>>      unsigned m_column : 31;
>>> 
>>> If you s/31/30/ here you should be able to get better big packing here.
>> 
>> You are totally right. However the change is big as-is so I didn't want to pollute it with an orthogonal - yet neat change. That will be moved to a follow-up bug.
> 
> A FIXME would be good.

I was going to reap this optimization directly after which is why I didn't want to put the FIXME. Sorry for being unclear about me considering the optimization. I filed bug 85291 to track that and assigned it to me.

>>>> Source/WebCore/rendering/RenderTableRow.h:93
>>>> +    unsigned m_rowIndex : 31;
>>> 
>>> Don't think we need to make this a bitfield now that it doesn't save us any memory.
>> 
>> You are right again but I would rather keep it for 2 reasons: symmetry with the column case and also as it keeps one bit for future optimization.
> 
> Meh. Up to you, but I don't think it makes sense to clutter header files with bitfields that aren't accomplishing anything. Symmetry with the column case doesn't really buy us anything since we clamp the value anyways and it doesn't let us share more code or anything.

I really would prefer to keep it this way as it means we also keep the existing testing (that we would have to throw away if we remove the bitfield, then we would also have to re-do if we add it back).

Btw, we may clamp the value but most of the table code is manipulating unsigned without consideration of whether they will be truncated at the cell / row level, which would lead to bad behaviors. This is also the reason we CRASH() if we will overflow.