[Webkit-unassigned] [Bug 25144] [patch] Adding QWebHistory::addItem

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25144


zecke at selfish.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29417|review?                     |review-
               Flag|                            |




------- Comment #2 from zecke at selfish.org  2009-04-11 20:32 PDT -------
(From update of attachment 29417)
First of all welcome and a '-' is not as bad as it might seem like. There are
some beginner issues though.


I think the API addition does make sense, so it is about technical issues as
this point. The comments are inline with the patch.




> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 42423)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> +2009-04-11  Mohammed Sameer  <set EMAIL_ADDRESS environment variable>

You should set EMAIL_ADDRESS in your environment or change it here.


> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +	[Qt] Added QWebHistory::addItem that allows adding history items
> +	to the page BackForwardList

Please use the eight spaces, tabs will be rejected by a post commit hook.



>  /*!
> +  Adds an item to the history.
> +
> +  Please note that this will "trash" any forward items

Some more documentation is needed. E.g. a @since 4.6 tag, description of the
parameters (e.g. what timezone is lastVisited), and maybe even a unit test,
showing the behavior (specially the trashing one).

> +*/
> +void QWebHistory::addItem(const QString &url, const QString &title, const QDateTime &lastVisited)
> +{
> +  QByteArray utf8 = url.toUtf8();
> +  WebCore::String Url(utf8.constData(), utf8.length());
> +  utf8 = title.toUtf8();
> +  WebCore::String Title(utf8.constData(), utf8.length());
> +  d->lst->addItem(WebCore::HistoryItem::create(Url, Title, lastVisited.toTime_t()));
> +}
>

You can write all this as:

d->lst->addItem(WebCore::HistoryItem::create(String(url), String(title),
last...);

Why this approach is better:
    - Less code
    - The conversion to Utf8 takes time and extra allocations and your import
into String is not losless... This c'tor is ascii only and will lose any non
ascii charachters.

Style:
    - Please use 4 spaces as of our CodingStyle guidelines on webkit.org
    - Please use a lowercase letter for the start of variable names


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.