Make WebHistory more type safe
Created attachment 212387 [details] Patch
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?
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
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.
Created attachment 212401 [details] Patch
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>>>"
Created attachment 212405 [details] Patch
Comment on attachment 212405 [details] Patch r=me
Comment on attachment 212405 [details] Patch Clearing flags on attachment: 212405 Committed r156321: <http://trac.webkit.org/changeset/156321>
All reviewed patches have been landed. Closing bug.