Bug 121801 – Make WebHistory more type safe

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Patrick R. Gansterer]

Make WebHistory more type safe

[Patrick R. Gansterer]

Created attachment 212387 [details]
Patch

[Brent Fulgham]

Comment on attachment 212387 [details]
Patch

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

I think the general idea of this change is good, but I don't agree with removing some of these methods, as they may be used by other WebKit clients.

> Source/WebKit/win/WebHistory.cpp:322
> +    DateToEntriesMap::iterator found = m_entriesByDate.find(dateKey(calendarDate));

How about 'auto found = ...'?

> Source/WebKit/win/WebHistory.cpp:343
> +        entriesForDate.at(i).copyRefTo(&items[i]);

Why use the "at(i)" syntax here? Is there some benefit over the square-bracket operator?

> Source/WebKit/win/WebHistory.h:-141
> -    HRESULT insertItem(IWebHistoryItem* entry, DateKey);

We don't want to get rid of the insertItem method.  I think "addItemToDateCaches" should be implemented in terms of insertItem.

> Source/WebKit/win/WebHistory.h:-143
> -    bool findKey(DateKey*, CFAbsoluteTime forDay);

Why don't we have 'bool findKey(DateKey*, DATE)'?  Why don't we want to keep this?

[Patrick R. Gansterer]

Comment on attachment 212387 [details]
Patch

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

>> Source/WebKit/win/WebHistory.cpp:322
>> +    DateToEntriesMap::iterator found = m_entriesByDate.find(dateKey(calendarDate));
> 
> How about 'auto found = ...'?

i copy&pasted it from other places, but i'm ok with the auto here too

>> Source/WebKit/win/WebHistory.cpp:343
>> +        entriesForDate.at(i).copyRefTo(&items[i]);
> 
> Why use the "at(i)" syntax here? Is there some benefit over the square-bracket operator?

no, i'll fix it

>> Source/WebKit/win/WebHistory.h:-141
>> -    HRESULT insertItem(IWebHistoryItem* entry, DateKey);
> 
> We don't want to get rid of the insertItem method.  I think "addItemToDateCaches" should be implemented in terms of insertItem.

it's private, so there should be nobody able to use it

>> Source/WebKit/win/WebHistory.h:-143
>> -    bool findKey(DateKey*, CFAbsoluteTime forDay);
> 
> Why don't we have 'bool findKey(DateKey*, DATE)'?  Why don't we want to keep this?

because we use the iterator now everywhere -> not needed

[Brent Fulgham]

Comment on attachment 212387 [details]
Patch

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

>>> Source/WebKit/win/WebHistory.h:-141
>>> -    HRESULT insertItem(IWebHistoryItem* entry, DateKey);
>> 
>> We don't want to get rid of the insertItem method.  I think "addItemToDateCaches" should be implemented in terms of insertItem.
> 
> it's private, so there should be nobody able to use it

Ah -- I missed that.  Okay -- never mind.

>>> Source/WebKit/win/WebHistory.h:-143
>>> -    bool findKey(DateKey*, CFAbsoluteTime forDay);
>> 
>> Why don't we have 'bool findKey(DateKey*, DATE)'?  Why don't we want to keep this?
> 
> because we use the iterator now everywhere -> not needed

Seems fine then.

[Patrick R. Gansterer]

Created attachment 212401 [details]
Patch

[Brent Fulgham]

Comment on attachment 212401 [details]
Patch

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

Looks good.  I did notice a couple of minor things that I missed last time.  The only one I really care about is the naming of the last visited time (I think this should stay as "lastVisitedTime", not be written as "entryTime".

You can fix this while landing, or you can upload a final patch and I will r+ that.

> Source/WebKit/win/WebHistory.cpp:590
> +    DATE entryTime;

I just noticed that this should be "lastVisitedTime".

> Source/WebKit/win/WebHistory.cpp:595
>          return E_FAIL;

This isn't your doing, but I wonder if attempting to remove an entry that is already not in the entry queue should be considered failure.  Seems like a safe "success" case to me.

> Source/WebKit/win/WebHistory.cpp:601
> +        if (entriesForDate.at(i) == entry)

We don't tend to use the "at()" method, but this is okay.

> Source/WebKit/win/WebHistory.cpp:625
> +        Vector<COMPtr<IWebHistoryItem> > entries(1);

You don't need the space here "> >" can be ">>" under C++11. :-)

> Source/WebKit/win/WebHistory.cpp:664
> +        if (FAILED(entriesForDate.at(mid)->lastVisitedTimeInterval(&itemTime)))

... another "at()".

> Source/WebKit/win/WebHistory.h:120
> +    typedef HashMap<DateKey, Vector<COMPtr<IWebHistoryItem> > > DateToEntriesMap;

You can write this as "...IWebHistoryItem>>>"

[Patrick R. Gansterer]

Created attachment 212405 [details]
Patch

[Brent Fulgham]

Comment on attachment 212405 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 212405 [details]
Patch

Clearing flags on attachment: 212405

Committed r156321: <http://trac.webkit.org/changeset/156321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.