Bug 235646

<rdar://problem/88078376>

Created attachment 450031 [details] Patch

Created attachment 450034 [details] Patch

Created attachment 450038 [details] Patch

Created attachment 450042 [details] Patch

(In reply to Tyler Wilcock from comment #3) > Created attachment 450034 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ a/Source/WebCore/accessibility/AXObjectCache.cpp + m_estimatedLoadingProgress = newProgressValue; Why estimated? This is the best we are going to know, right? So for us is the truth. --- a/Source/WebCore/accessibility/AXObjectCache.h +++ a/Source/WebCore/accessibility/AXObjectCache.h + void updateEstimatedLoadingProgress(double); + void loadingProgressFinished() { updateEstimatedLoadingProgress(1); } + double estimatedLoadingProgress() const { return m_estimatedLoadingProgress; } consider updateLoadingProgress, loadingFinished and loadingProgress instead. --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h - double estimatedLoadingProgress() const override { return doubleAttributeValue(AXPropertyName::EstimatedLoadingProgress); } + double estimatedLoadingProgress() const override { return tree()->estimatedLoadingProgress(); } Would get rid of "estimated". We are not estimating anything. --- a/Source/WebCore/page/Page.cpp +++ a/Source/WebCore/page/Page.cpp +void Page::progressEstimateChanged(Frame& frameWithProgressUpdate) const +{ + if (auto* document = frameWithProgressUpdate.document()) { + if (auto* axObjectCache = document->axObjectCache()) + axObjectCache->updateEstimatedLoadingProgress(progress().estimatedProgress()); + } +} + +void Page::progressFinished(Frame& frameWithCompletedProgress) const +{ + if (auto* document = frameWithCompletedProgress.document()) { + if (auto* axObjectCache = document->axObjectCache()) + axObjectCache->loadingProgressFinished(); + } +} + Perhaps we want to do document->axObjectCacheIfExist() instead? Because I think document->axObjectCache() creates it if it doesn't exist, but I'm going by memory here.

(In reply to Andres Gonzalez from comment #6) > (In reply to Tyler Wilcock from comment #3) > > Created attachment 450034 [details] > > Patch > > --- a/Source/WebCore/accessibility/AXObjectCache.cpp > +++ a/Source/WebCore/accessibility/AXObjectCache.cpp > > + m_estimatedLoadingProgress = newProgressValue; > > Why estimated? This is the best we are going to know, right? So for us is > the truth. > > --- a/Source/WebCore/accessibility/AXObjectCache.h > +++ a/Source/WebCore/accessibility/AXObjectCache.h > > + void updateEstimatedLoadingProgress(double); > + void loadingProgressFinished() { updateEstimatedLoadingProgress(1); } > + double estimatedLoadingProgress() const { return > m_estimatedLoadingProgress; } > > consider updateLoadingProgress, loadingFinished and loadingProgress instead. > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > - double estimatedLoadingProgress() const override { return > doubleAttributeValue(AXPropertyName::EstimatedLoadingProgress); } > + double estimatedLoadingProgress() const override { return > tree()->estimatedLoadingProgress(); } > > Would get rid of "estimated". We are not estimating anything. > > --- a/Source/WebCore/page/Page.cpp > +++ a/Source/WebCore/page/Page.cpp > > +void Page::progressEstimateChanged(Frame& frameWithProgressUpdate) const > +{ > + if (auto* document = frameWithProgressUpdate.document()) { > + if (auto* axObjectCache = document->axObjectCache()) > + > axObjectCache->updateEstimatedLoadingProgress(progress(). > estimatedProgress()); > + } > +} > + > +void Page::progressFinished(Frame& frameWithCompletedProgress) const > +{ > + if (auto* document = frameWithCompletedProgress.document()) { > + if (auto* axObjectCache = document->axObjectCache()) > + axObjectCache->loadingProgressFinished(); > + } > +} > + > Perhaps we want to do document->axObjectCacheIfExist() instead? Because I > think document->axObjectCache() creates it if it doesn't exist, but I'm > going by memory here. Ah, axObjectCacheIfExists() is definitely what we want. Thanks! I chose to name things like "estimatedLoadingProgress" instead of "loadingProgress" to be consistent with that naming in ProgressTracker, and in the AXCoreObject interface, which has a method called estimatedLoadingProgress. I do agree that "loading progress" is better naming than "estimated loading progress", so I'm going to fix this by renaming the AXCoreObject interface method.

Created attachment 450054 [details] Patch

Created attachment 450071 [details] Patch

Created attachment 450076 [details] Patch

Comment on attachment 450076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450076&action=review > Source/WebCore/accessibility/AXObjectCache.h:529 > + double m_loadingProgress { 0.25 }; how come default is .25 > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:363 > + void updateLoadingProgress(double newProgress); not necessary to name newProgress here > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:416 > + double m_loadingProgress { 0.25 }; defaults .25?

(In reply to chris fleizach from comment #11) > Comment on attachment 450076 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450076&action=review > > > Source/WebCore/accessibility/AXObjectCache.h:529 > > + double m_loadingProgress { 0.25 }; > > how come default is .25 > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:363 > > + void updateLoadingProgress(double newProgress); > > not necessary to name newProgress here > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:416 > > + double m_loadingProgress { 0.25 }; > > defaults .25? The default is mostly arbitrary so long as it's not 100% since it will be overwritten as the page loads / reloads. I figured that by the time the cache / tree is created, we can guess the page will be at least 25% loaded, but no science there. In my testing this default is never actually used, but we should default initialize the value to something if not this.

(In reply to Tyler Wilcock from comment #12) > (In reply to chris fleizach from comment #11) > > Comment on attachment 450076 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=450076&action=review > > > > > Source/WebCore/accessibility/AXObjectCache.h:529 > > > + double m_loadingProgress { 0.25 }; > > > > how come default is .25 > > > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:363 > > > + void updateLoadingProgress(double newProgress); > > > > not necessary to name newProgress here > > > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:416 > > > + double m_loadingProgress { 0.25 }; > > > > defaults .25? > The default is mostly arbitrary so long as it's not 100% since it will be > overwritten as the page loads / reloads. I figured that by the time the > cache / tree is created, we can guess the page will be at least 25% loaded, > but no science there. In my testing this default is never actually used, but > we should default initialize the value to something if not this. Ok if it doesn't matter, might be less confusing to just leave as zero (for other programmers looking at this)

Created attachment 450085 [details] Patch

Created attachment 450086 [details] Patch

> Ok if it doesn't matter, might be less confusing to just leave as zero (for > other programmers looking at this) OK, done.

Committed r288674 (246480@main): <https://commits.webkit.org/246480@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450086 [details].

This broke !ENABLE(ACCESSIBILITY) builds because updateLoadingProgress wasn't added to in the block of inlined methods below. Following up in with a patch in https://bugs.webkit.org/show_bug.cgi?id=235727

Sorry about that. Thanks for fixing it.

(In reply to Tyler Wilcock from comment #19) > Sorry about that. Thanks for fixing it. No worries it happens since none of the EWS bots are !ENABLE(ACCESSIBILITY).