|
|
Created:
3 years, 10 months ago by aleventhal Modified:
3 years, 10 months ago CC:
chromium-reviews, aboxhall, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, blink-reviews, nektarios, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, dmazzoni Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSome cleanup of AxObjectCacheImpl.cpp
- Dry up uses of getAXID() (save result in const)
- Rename getAXID() to getOrCreateAXID to show it's more than a simple getter
(which usually don't have side effects).
- Remove unused method ::rootObject().
- Change ASSERT to DCHECK
BUG=none
R=dmazzoni
Review-Url: https://codereview.chromium.org/2696213002
Cr-Commit-Position: refs/heads/master@{#452668}
Committed: https://chromium.googlesource.com/chromium/src/+/ac41985973c63800fd911fc6e93921676462e988
Patch Set 1 #
Total comments: 4
Patch Set 2 : Stylistic changes #Patch Set 3 : Fix compilation/refactoring error #Patch Set 4 : Attempt to fix try jobs #Patch Set 5 : Rebasing #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Messages
Total messages: 62 (43 generated)
Thanks, these changes look good https://codereview.chromium.org/2696213002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp (right): https://codereview.chromium.org/2696213002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:367: const AXID axId = getOrCreateAXID(newObj); nit: I think I'd prefer axID for consistency. I don't have a strong preference but if we change it I'd prefer to change everything, like getOrCreateAXId(), etc. https://codereview.chromium.org/2696213002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:391: const AXID axid = getOrCreateAXID(newObj); I'm also fine with all lowercase. Just make it consistent. axID and axid are both fine. axId is not as consistent. https://codereview.chromium.org/2696213002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:526: const AXID prevObjID = obj->axObjectID(); nit: maybe existingObjID instead of prev, because previous implies it might change, whereas existing implies that it might already exist and if not we'll create it. But just a suggestion, won't push it if you disagree. https://codereview.chromium.org/2696213002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:532: const AXID newObjID = platformGenerateAXID(); As long as you're cleaning it up, rename platformGenerateAXID - get rid of the word "platform" and maybe even just fold it into this function. Historically there were different functions to get AXIDs on each platform in WebKit, where Chromium was one "platform".
Review nits addressed
The CQ bit was checked by dmazzoni@chromium.org
lgtm
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
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Looks like you'll need to update WebAXObject.cpp too. No need for another review from me, but you'll need to add someone from the OWNERS file for WebAXObject.cpp, I believe. ../../third_party/WebKit/Source/web/WebAXObject.cpp:157:37: error: 'class blink::AXObjectCacheImpl' has no member named 'platformGenerateAXID' return m_private->axObjectCache().platformGenerateAXID();
On 2017/02/17 05:38:26, dmazzoni wrote: > Looks like you'll need to update WebAXObject.cpp too. > No need for another review from me, but you'll need to add > someone from the OWNERS file for WebAXObject.cpp, I > believe. > > ../../third_party/WebKit/Source/web/WebAXObject.cpp:157:37: error: 'class > blink::AXObjectCacheImpl' has no member named 'platformGenerateAXID' > return m_private->axObjectCache().platformGenerateAXID(); LGTM
The CQ bit was checked by aleventhal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2696213002/#ps40001 (title: "Fix compilation/refactoring error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/17 05:38:26, dmazzoni wrote: > Looks like you'll need to update WebAXObject.cpp too. > No need for another review from me, but you'll need to add > someone from the OWNERS file for WebAXObject.cpp, I > believe. > > ../../third_party/WebKit/Source/web/WebAXObject.cpp:157:37: error: 'class > blink::AXObjectCacheImpl' has no member named 'platformGenerateAXID' > return m_private->axObjectCache().platformGenerateAXID(); Done and done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by aleventhal@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by aleventhal@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 checked by aleventhal@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by aleventhal@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by aleventhal@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 aleventhal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2696213002/#ps60001 (title: "Attempt to fix try jobs")
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
Failed to apply patch for third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp: While running git apply --index -p1; error: patch failed: third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:555 error: third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp: patch does not apply Patch: third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp Index: third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp diff --git a/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp b/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp index 24472e9113c8d3df1c84301c02245d794aca0b2e..73aefdc47075d361be8fcbab51a87390b785451f 100644 --- a/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp +++ b/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp @@ -100,7 +100,9 @@ AXObjectCacheImpl::AXObjectCacheImpl(Document& document) &AXObjectCacheImpl::notificationPostTimerFired) {} AXObjectCacheImpl::~AXObjectCacheImpl() { - ASSERT(m_hasBeenDisposed); +#if DCHECK_IS_ON() + DCHECK(m_hasBeenDisposed); +#endif } void AXObjectCacheImpl::dispose() { @@ -190,7 +192,7 @@ AXObject* AXObjectCacheImpl::get(LayoutObject* layoutObject) { return 0; AXID axID = m_layoutObjectMapping.get(layoutObject); - ASSERT(!HashTraits<AXID>::isDeletedValue(axID)); + DCHECK(!HashTraits<AXID>::isDeletedValue(axID)); if (!axID) return 0; @@ -220,10 +222,10 @@ AXObject* AXObjectCacheImpl::get(Node* node) { layoutObject = nullptr; AXID layoutID = layoutObject ? m_layoutObjectMapping.get(layoutObject) : 0; - ASSERT(!HashTraits<AXID>::isDeletedValue(layoutID)); + DCHECK(!HashTraits<AXID>::isDeletedValue(layoutID)); AXID nodeID = m_nodeObjectMapping.get(node); - ASSERT(!HashTraits<AXID>::isDeletedValue(nodeID)); + DCHECK(!HashTraits<AXID>::isDeletedValue(nodeID)); if (layoutObject && nodeID && !layoutID) { // This can happen if an AXNodeObject is created for a node that's not @@ -247,7 +249,7 @@ AXObject* AXObjectCacheImpl::get(AbstractInlineTextBox* inlineTextBox) { return 0; AXID axID = m_inlineTextBoxObjectMapping.get(inlineTextBox); - ASSERT(!HashTraits<AXID>::isDeletedValue(axID)); + DCHECK(!HashTraits<AXID>::isDeletedValue(axID)); if (!axID) return 0; @@ -362,12 +364,11 @@ AXObject* AXObjectCacheImpl::getOrCreate(Node* node) { AXObject* newObj = createFromNode(node); // Will crash later if we have two objects for the same node. - ASSERT(!get(node)); + DCHECK(!get(node)); - getAXID(newObj); + const AXID axID = getOrCreateAXID(newObj); - m_nodeObjectMapping.set(node, newObj->axObjectID()); - m_objects.set(newObj->axObjectID(), newObj); + m_nodeObjectMapping.set(node, axID); newObj->init(); newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored()); @@ -387,12 +388,11 @@ AXObject* AXObjectCacheImpl::getOrCreate(LayoutObject* layoutObject) { AXObject* newObj = createFromRenderer(layoutObject); // Will crash later if we have two objects for the same layoutObject. - ASSERT(!get(layoutObject)); + DCHECK(!get(layoutObject)); - getAXID(newObj); + const AXID axid = getOrCreateAXID(newObj); - m_layoutObjectMapping.set(layoutObject, newObj->axObjectID()); - m_objects.set(newObj->axObjectID(), newObj); + m_layoutObjectMapping.set(layoutObject, axid); newObj->init(); newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored()); @@ -409,25 +409,17 @@ AXObject* AXObjectCacheImpl::getOrCreate(AbstractInlineTextBox* inlineTextBox) { AXObject* newObj = createFromInlineTextBox(inlineTextBox); // Will crash later if we have two objects for the same inlineTextBox. - ASSERT(!get(inlineTextBox)); + DCHECK(!get(inlineTextBox)); - getAXID(newObj); + const AXID axid = getOrCreateAXID(newObj); - m_inlineTextBoxObjectMapping.set(inlineTextBox, newObj->axObjectID()); - m_objects.set(newObj->axObjectID(), newObj); + m_inlineTextBoxObjectMapping.set(inlineTextBox, axid); newObj->init(); newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored()); return newObj; } -AXObject* AXObjectCacheImpl::rootObject() { - if (!accessibilityEnabled()) - return 0; - - return getOrCreate(m_document); -} - AXObject* AXObjectCacheImpl::getOrCreate(AccessibilityRole role) { AXObject* obj = nullptr; @@ -455,12 +447,11 @@ AXObject* AXObjectCacheImpl::getOrCreate(AccessibilityRole role) { obj = nullptr; } - if (obj) - getAXID(obj); - else + if (!obj) return 0; - m_objects.set(obj->axObjectID(), obj); + getOrCreateAXID(obj); + obj->init(); return obj; } @@ -481,7 +472,7 @@ void AXObjectCacheImpl::remove(AXID axID) { if (!m_objects.take(axID)) return; - ASSERT(m_objects.size() >= m_idsInUse.size()); + DCHECK(m_objects.size() >= m_idsInUse.size()); } void AXObjectCacheImpl::remove(LayoutObject* layoutObject) { @@ -517,7 +508,7 @@ void AXObjectCacheImpl::remove(AbstractInlineTextBox* inlineTextBox) { m_inlineTextBoxObjectMapping.erase(inlineTextBox); } -AXID AXObjectCacheImpl::platformGenerateAXID() const { +AXID AXObjectCacheImpl::generateAXID() const { static AXID lastUsedID = 0; // Generate a new ID. @@ -532,20 +523,21 @@ AXID AXObjectCacheImpl::platformGenerateAXID() const { return objID; } -AXID AXObjectCacheImpl::getAXID(AXObject* obj) { +AXID AXObjectCacheImpl::getOrCreateAXID(AXObject* obj) { // check for already-assigned ID - AXID objID = obj->axObjectID(); - if (objID) { - ASSERT(m_idsInUse.contains(objID)); - return objID; + const AXID existingAXID = obj->axObjectID(); + if (existingAXID) { + DCHECK(m_idsInUse.contains(existingAXID)); + return existingAXID; } - objID = platformGenerateAXID(); + const AXID newAXID = generateAXID(); - m_idsInUse.insert(objID); - obj->setAXObjectID(objID); + m_idsInUse.insert(newAXID); + obj->setAXObjectID(newAXID); + m_objects.set(newAXID, obj); - return objID; + return newAXID; } void AXObjectCacheImpl::removeAXID(AXObject* object) { @@ -555,8 +547,8 @@ void AXObjectCacheImpl::removeAXID(AXObject* object) { AXID objID = object->axObjectID(); if (!objID) return; - ASSERT(!HashTraits<AXID>::isDeletedValue(objID)); - ASSERT(m_idsInUse.contains(objID)); + DCHECK(!HashTraits<AXID>::isDeletedValue(objID)); + DCHECK(m_idsInUse.contains(objID)); object->setAXObjectID(0); m_idsInUse.remove(objID); @@ -646,7 +638,7 @@ void AXObjectCacheImpl::notificationPostTimerFired(TimerBase*) { AXLayoutObject* layoutObj = toAXLayoutObject(obj); LayoutObject* layoutObject = layoutObj->getLayoutObject(); if (layoutObject && layoutObject->view()) - ASSERT(!layoutObject->view()->layoutState()); + DCHECK(!layoutObject->view()->layoutState()); } #endif @@ -868,7 +860,7 @@ void AXObjectCacheImpl::updateTreeIfElementIdIsAriaOwned(Element* element) { // still an owner. if (isAriaOwned(axElement)) { AXObject* ownedParent = getAriaOwnedParent(axElement); - ASSERT(ownedParent); + DCHECK(ownedParent); childrenChanged(ownedParent); return; }
The CQ bit was checked by aleventhal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2696213002/#ps80001 (title: "Rebasing")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by aleventhal@chromium.org
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
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...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 aleventhal@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 aleventhal@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 aleventhal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2696213002/#ps120001 (title: "Rebase")
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": 120001, "attempt_start_ts": 1487891261995310, "parent_rev": "fbd566a07bb73d4321780c44ac5ed0fb2ef342e0", "commit_rev": "ac41985973c63800fd911fc6e93921676462e988"}
Message was sent while issue was closed.
Description was changed from ========== Some cleanup of AxObjectCacheImpl.cpp - Dry up uses of getAXID() (save result in const) - Rename getAXID() to getOrCreateAXID to show it's more than a simple getter (which usually don't have side effects). - Remove unused method ::rootObject(). - Change ASSERT to DCHECK BUG=none R=dmazzoni ========== to ========== Some cleanup of AxObjectCacheImpl.cpp - Dry up uses of getAXID() (save result in const) - Rename getAXID() to getOrCreateAXID to show it's more than a simple getter (which usually don't have side effects). - Remove unused method ::rootObject(). - Change ASSERT to DCHECK BUG=none R=dmazzoni Review-Url: https://codereview.chromium.org/2696213002 Cr-Commit-Position: refs/heads/master@{#452668} Committed: https://chromium.googlesource.com/chromium/src/+/ac41985973c63800fd911fc6e939... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ac41985973c63800fd911fc6e939... |