Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560673002:
Change key of m_styleReattachDataMap to Node instead of Element (Closed)
DescriptionChange key of m_styleReattachDataMap to Node instead of Element
This patch changed the m_styleReattachDataMap key from Element to Node
which is needed so that we can store Text nodes and their
nextTextSibling in this map as well. Not possible if key is Element.
Need to store Text nodes as we need to pass information about
the nextTextSibling between Text::recalcTextStyle and
Text::rebuildTextLayoutTree.
BUG=595137
Committed: https://crrev.com/ce6cb90916b41f6ef17ae157812ded93fdbd8812
Cr-Commit-Position: refs/heads/master@{#436873}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add comment #Patch Set 3 : Add comment and check #Patch Set 4 : Add comment and DCHECK #
Messages
Total messages: 36 (23 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nainar@chromium.org changed reviewers: + bugsnash@chromium.org
PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
On 2016/12/06 at 23:32:13, nainar wrote: > PTAL? > > Thanks! lgtm. Please include in the description why we want to be able to store Text nodes
Description was changed from ========== Change key of m_styleReattachDataMap to Node instead of Element This patch changed the m_styleReattachDataMap key from Element to Node which is needed so that we can store Text nodes and their nextTextSibling in this map as well. Not possible if key is Element. BUG=595137 ========== to ========== Change key of m_styleReattachDataMap to Node instead of Element This patch changed the m_styleReattachDataMap key from Element to Node which is needed so that we can store Text nodes and their nextTextSibling in this map as well. Not possible if key is Element. Need to store Text nodes as we need to pass information about the nextTextSibling between Text::recalcTextStyle and Text::rebuildTextLayoutTree. BUG=595137 ==========
Done. Sending to sashab@ for OWNERS.
nainar@chromium.org changed reviewers: + sashab@chromium.org
I don't think this has any performance or memory implications since it's just a pointer type change and Node is a parent class of Element. https://codereview.chromium.org/2560673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2560673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.h:1442: Add comment here explaining what this hash map stores.
Other than adding a comment to make future changes like this easier, lgtm
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2560673002/#ps20001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Great great comment. Did you want to add a DCHECK somewhere that ensures its only an Element or Text object? Or are all types of Nodes OK? You can do this in a follow up patch.
On 2016/12/07 at 02:49:43, sashab wrote: > Great great comment. Did you want to add a DCHECK somewhere that ensures its only an Element or Text object? Or are all types of Nodes OK? You can do this in a follow up patch. I don't think this comment is very clear and repeats information that is self documented in the code. Perhaps “Used to stash information generated in the style resolution phase that is required in the layout tree construction phase” I think that's all the info you need to know to understand this HashMap - want more details about what's in it look at the code
On 2016/12/07 at 02:52:39, Bugs Nash wrote: > On 2016/12/07 at 02:49:43, sashab wrote: > > Great great comment. Did you want to add a DCHECK somewhere that ensures its only an Element or Text object? Or are all types of Nodes OK? You can do this in a follow up patch. > > I don't think this comment is very clear and repeats information that is self documented in the code. Perhaps “Used to stash information generated in the style resolution phase that is required in the layout tree construction phase” I think that's all the info you need to know to understand this HashMap - want more details about what's in it look at the code Also yeah if the intention is to strictly only allow Element or Text that should be DCHECK'd
The CQ bit was unchecked by nainar@chromium.org
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2560673002/#ps60001 (title: "Add comment and DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481088698395210,
"parent_rev": "d3e0319d7a89bbfea9db837da3736a9117ecb0d3", "commit_rev":
"6a33247ada8b6f4b9f55512e2f30b41befd5a8eb"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change key of m_styleReattachDataMap to Node instead of Element This patch changed the m_styleReattachDataMap key from Element to Node which is needed so that we can store Text nodes and their nextTextSibling in this map as well. Not possible if key is Element. Need to store Text nodes as we need to pass information about the nextTextSibling between Text::recalcTextStyle and Text::rebuildTextLayoutTree. BUG=595137 ========== to ========== Change key of m_styleReattachDataMap to Node instead of Element This patch changed the m_styleReattachDataMap key from Element to Node which is needed so that we can store Text nodes and their nextTextSibling in this map as well. Not possible if key is Element. Need to store Text nodes as we need to pass information about the nextTextSibling between Text::recalcTextStyle and Text::rebuildTextLayoutTree. BUG=595137 Committed: https://crrev.com/ce6cb90916b41f6ef17ae157812ded93fdbd8812 Cr-Commit-Position: refs/heads/master@{#436873} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ce6cb90916b41f6ef17ae157812ded93fdbd8812 Cr-Commit-Position: refs/heads/master@{#436873} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
