[webkit-reviews] review granted: [Bug 125431] All observable PageLoadState properties should change in an atomic fashion, with properly nested change notifications : [Attachment 218719] Add PageLoadState::commitChangesIfNeeded and Change objects to manage committing changes

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 9 10:41:51 PST 2013


Darin Adler <darin at apple.com> has granted mitz at webkit.org <mitz at webkit.org>'s
request for review:
Bug 125431: All observable PageLoadState properties should change in an atomic
fashion, with properly nested change notifications
https://bugs.webkit.org/show_bug.cgi?id=125431

Attachment 218719: Add PageLoadState::commitChangesIfNeeded and Change objects
to manage committing changes
https://bugs.webkit.org/attachment.cgi?id=218719&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218719&action=review


The basic approach here seems good. Making sure we don’t forget to commit
changes by requiring a change object to make the changes is a clever technique.
To me the change object seems like a change delivery deferral object, more like
PageGroupLoadDeferrer than anything else. So it’s a little irritating that it’s
called “a change”; that doesn’t make it clear that the timing of its
destruction is important and observable.

Also kind of sad that the change object doesn’t help us get m_needsCommitting
set correctly.

I am not so happy with the unpredictable timing for committing changes when
they happen in functions that lack an explicit call to commitChangesIfNeeded. I
assume that there is some good reason that this is guaranteed to be OK, but
it’s not entirely obvious to me from reading the code. Deferring to the end of
the last function that was making a change seems sort of “too far to defer” and
yet “not deferred far enough”, but maybe that’s just an unfounded or aesthetic
worry.

> Source/WebKit2/UIProcess/PageLoadState.cpp:32
> +    : m_data()

It seems to me that making m_data const is not the right way to express its
semantics. Can we leave it non-const and just take care to modify it only in
the commit function? I would suggest that instead of using const we give this a
name that makes it clear you should not modify it. It is equally incorrect to
modify m_pendingData without setting m_needsCommitting as to modify m_data, so
I think the const does not help us enough here; I could argue that we need
m_pendingData to be const.

If you decide that you like the const trick, there’s a different const trick we
can play with m_pendingData, where we make a function that you have to call to
get a non-const reference to it, and that call sets m_needsCommitting so you
can’t forget to do that.

Maybe m_uncommittedState and m_committedState are good names. And maybe
m_needsCommitting could be renamed m_mayHaveUncommittedChanges, because the
boolean doesn’t really indicate whether we need committing. Although I suppose
it’s like needsDisplay and such so maybe doesn’t need a name change.

> Source/WebKit2/UIProcess/PageLoadState.cpp:34
> +    , m_changeCount(0)

I’m not sure that “change count” is a good name for this because that sounds
like a count of the total number of changes to page load state. To me it seems
to be a count of the number of page load changes that are currently being done,
which the names does not make clear. Maybe “outstanding” or “underway” in the
name will help.

> Source/WebKit2/UIProcess/PageLoadState.cpp:37
> +    m_pendingData.state = State::Finished;
> +    m_pendingData.estimatedProgress = 0;

This code could go in a PageLoadState::Data constructor and then we could avoid
both the explicit construction of m_data above and avoid these lines of code.

> Source/WebKit2/UIProcess/PageLoadState.cpp:38
> +    *const_cast<Data*>(&m_data) = m_pendingData;

If we keep it, no need to involve a pointer in this type cast:

    const_cast<Data&>(m_data) = m_pendingData;

> Source/WebKit2/UIProcess/PageLoadState.cpp:69
> +void PageLoadState::commitChangesIfNeeded()

I notice that the changes are committed in a particular order. Did we put
specific thought into the ordering? If so, please add a brief comment
explaining the thinking, including the fact that the “did” ordering is the
reverse of the “will”.

> Source/WebKit2/UIProcess/PageLoadState.cpp:76
> +    bool titleChanged = m_pendingData.title != m_data.title;

The other comparisons below have m_data on left and m_pendingData on the right.
Would be nice to be consistent.

> Source/WebKit2/UIProcess/PageLoadState.cpp:90
> +    *const_cast<Data*>(&m_data) = m_pendingData;

Same const_cast pointer/reference thing here.

> Source/WebKit2/UIProcess/PageLoadState.cpp:107
>      setState(State::Finished);

Strange that this is done through a function while all the rest is done by
setting members of m_pendingData directly.

> Source/WebKit2/UIProcess/PageLoadState.cpp:118
> -    m_pendingAPIRequestURL = String();
> -    m_provisionalURL = String();
> -    m_url = String();
> +    m_pendingData.pendingAPIRequestURL = String();
> +    m_pendingData.provisionalURL = String();
> +    m_pendingData.url = String();
>  
> -    m_unreachableURL = String();
> +    m_pendingData.unreachableURL = String();
>      m_lastUnreachableURL = String();
>  
> -    callObserverCallback(&Observer::willChangeTitle);
> -    m_title = String();
> -    callObserverCallback(&Observer::didChangeTitle);
> -
> -    callObserverCallback(&Observer::willChangeEstimatedProgress);
> -    m_estimatedProgress = 0;
> -    callObserverCallback(&Observer::didChangeEstimatedProgress);
> +    m_pendingData.title = String();
> +
> +    m_pendingData.estimatedProgress = 0;

Maybe PageLoadState::Data should have a reset member function for this.

> Source/WebKit2/UIProcess/PageLoadState.cpp:126
> -String PageLoadState::activeURL() const
> +String PageLoadState::activeURL(const Data& data)

Seems like this could be a member of PageLoadState::Data.

> Source/WebKit2/UIProcess/PageLoadState.cpp:157
>  // Always start progress at initialProgressValue. This helps provide
feedback as
>  // soon as a load starts.
>  
>  static const double initialProgressValue = 0.1;

I’d like this better at the top of the file, and I’d like the comment better as
a single line.

> Source/WebKit2/UIProcess/PageLoadState.cpp:331
>  void PageLoadState::setState(State state)

This function is not helpful any more. I suggest inlining it at the call sites.


> Source/WebKit2/UIProcess/PageLoadState.cpp:335
> +    m_pendingData.state = state;

Why do we not set m_needsCommitting here? Couldn’t isLoading or activeURL
change based on a state change? This question only arises because we left this
as a separate function.

> Source/WebKit2/UIProcess/PageLoadState.h:62
> +	   WTF_MAKE_NONCOPYABLE(Change);

Since we have a member that is a reference, we don’t need to use this macro. I
like the economy of not using it.

> Source/WebKit2/UIProcess/PageLoadState.h:67
> +	   Change(Change&& other)
> +	       : m_pageLoadState(other.m_pageLoadState)
> +	   {
> +	   }

I think the compiler will write this move constructor for us, exactly like
this, as long as we don’t declare any move or copy constructors. Unless maybe
explicitly defining the private constructor inhibits that? I suggest trying
without this and I expect we will see that it “just works”.

> Source/WebKit2/UIProcess/PageLoadState.h:75
> +	   friend class PageLoadState;

I suspect that this is unneeded. I seem to recall that a nested class always
bestows friendship on the class it is nested in.

> Source/WebKit2/UIProcess/PageLoadState.h:89
> +    Change change() { return Change(*this); }

Since the constructor of Change is not explicit, I don’t think we need to
invoke the constructor explicitly here. If we do want to be explicit, then I
suggest we use the “explicit” keyword when declaring the constructor.

> Source/WebKit2/UIProcess/PageLoadState.h:90
> +    void commitChangesIfNeeded();

This function doesn’t need “if needed” in its name. It always commits changes
if they exist and never commits changes that don’t exist, so I think “commit
changes” is a good name for it.

> Source/WebKit2/UIProcess/PageLoadState.h:92
> +    void reset(const Change&);

Since the Change object argument is debug-only it would be a little nicer if we
could have found an idiom that optimizes the pointer out entirely. Maybe the
assertion would be in an inline function and then the function to do the real
work would not take the Change object.

> Source/WebKit2/UIProcess/PageLoadState.h:131
> +    void beginChange() { m_changeCount++; }

I’d use pre-increment since that’s more idiomatic in our code (for reasons that
are presumably obsolete).

> Source/WebKit2/UIProcess/PageLoadState.h:154
> +    static String activeURL(const Data&);
> +    static double estimatedProgress(const Data&);

I think both of these would be good as members of Data.

> Source/WebKit2/UIProcess/PageLoadState.h:159
> +    String m_lastUnreachableURL;

It’s not obvious that this is part of m_pendingData and needs to participate in
the idiom where you need to set m_needsCommitting if you change it. One way to
possibly make that clearer would be to have a struct for uncommitted data that
derives from the struct for committed data and includes this field.

> Source/WebKit2/UIProcess/PageLoadState.h:162
> +    int m_changeCount;

I suggest considering unsigned instead of int.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:604
> +    auto change = m_pageLoadState.change();
> +
> +    m_pageLoadState.setPendingAPIRequestURL(change, url);

I think it’s unclear that these change objects are RAII objects and people
might overlook the importance of their lifetimes.

Should I be concerned that this change will be committed all the way at the end
of this loadURL function rather than earlier, say right after this function
call? To put this another way, I would be tempted to write this:

    m_pageLoadState.setPendingAPIRequestURL(m_pageLoadState.change(), url);

Is there something wrong with writing it that way in a case like this where
there is only a single change?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:688
>      if (!isValid())
>	   reattachToWebProcess();
>  
> -    m_pageLoadState.setUnreachableURL(unreachableURL);
> +    auto change = m_pageLoadState.change();
> +
> +    m_pageLoadState.setUnreachableURL(change, unreachableURL);

Strangely, in this function we do the work after calling reattachToWebProcess.
Inconsistent with the other cases above.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2420
> +    const String& pendingAPIRequestURL =
m_pageLoadState.pendingAPIRequestURL();
> +    auto change = m_pageLoadState.change();
> +
> +    if (request.url() != pendingAPIRequestURL)
> +	   m_pageLoadState.clearPendingAPIRequestURL(change);

I think the pendingAPIRequestURL local variable makes this code harder to read.
I suggest not using a local for it.


More information about the webkit-reviews mailing list