Bug 68321 – [EFL] Use C++ type cast instead of C style type cast

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Gyuyoung Kim]

Some codes are using C type cast though file format is .cpp. A patch for this bug will be submitted after fixing Bug 68231.

[Gyuyoung Kim]

Created attachment 108354 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 108354 [details]
Patch

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

I was going to comment on all occurrences which changed "Foo* p = ..." to "Foo *p = ..." but gave up about halfway into the patch.

> Source/WebKit/efl/ewk/ewk_auth_soup.cpp:82
> +    Ewk_Auth_Data *auth_data = static_cast<Ewk_Auth_Data*>(data);

Wrong '*' position.

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:106
> +    Ewk_Context_Menu_Item *item = static_cast<Ewk_Context_Menu_Item*>(malloc(sizeof(*item)));

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:689
> +    Ewk_Hit_Test *hit_test = static_cast<Ewk_Hit_Test*>(calloc(1, sizeof(Ewk_Hit_Test)));

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:210
> +    Ewk_View_Smart_Data *ptr = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(o))

The placement of the first '*' could be fixed here.

> Source/WebKit/efl/ewk/ewk_view.cpp:473
> +    Ewk_View_Smart_Data *sd = static_cast<Ewk_View_Smart_Data*>(data);

Wrong position for the first '*'.

> Source/WebKit/efl/ewk/ewk_view.cpp:481
> +    Ewk_View_Smart_Data *sd = static_cast<Ewk_View_Smart_Data*>(data);

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:727
> +    const Ewk_View_Smart_Class* api = (Ewk_View_Smart_Class*)(sc);

No C++ cast here.

[Lucas De Marchi]

(In reply to comment #2)
> (From update of attachment 108354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108354&action=review
> 
> I was going to comment on all occurrences which changed "Foo* p = ..." to "Foo *p = ..." but gave up about halfway into the patch.

Gyuyoung, you might want to apply the uncrustify config on top of this patch and check if it changed back the position of '*'.

[Gyuyoung Kim]

Created attachment 108441 [details]
Patch

[Gyuyoung Kim]

Sorry, I missed to fix '*' operator in my local changes despite running git pull. I run uncrustify.cfg in my files. Fortunately, I found remained wrong pointer operator placement. I fix them as well.

>> Source/WebKit/efl/ewk/ewk_view.cpp:727
>> +    const Ewk_View_Smart_Class* api = (Ewk_View_Smart_Class*)(sc);

> No C++ cast here.

I fix this line using reinterpret_cast because of different pointer casting.

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Lucas De Marchi]

Comment on attachment 108441 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:60
> +        * ewk/ewk_auth_soup.cpp:
> +        (_Ewk_Auth_Data::ewk_auth_soup_credentials_set):
> +        (_Ewk_Auth_Data::session_authenticate):
> +        * ewk/ewk_contextmenu.cpp:
> +        (ewk_context_menu_item_new):
> +        * ewk/ewk_frame.cpp:
> +        (_ewk_frame_smart_add):
> +        (ewk_frame_children_iterator_new):
> +        (ewk_frame_hit_test_new):
> +        * ewk/ewk_history.cpp:
> +        (_ewk_history_item_new):
> +        (ewk_history_item_title_get):
> +        (ewk_history_item_title_alternate_get):
> +        (ewk_history_item_uri_get):
> +        (ewk_history_item_uri_original_get):
> +        (ewk_history_new):
> +        * ewk/ewk_view.cpp:
> +        (_ewk_view_repaints_resize):
> +        (_ewk_view_scrolls_resize):
> +        (_ewk_view_on_focus_in):
> +        (_ewk_view_on_focus_out):
> +        (_ewk_view_on_mouse_wheel):
> +        (_ewk_view_on_mouse_down):
> +        (_ewk_view_on_mouse_up):
> +        (_ewk_view_on_mouse_move):
> +        (_ewk_view_on_key_down):
> +        (_ewk_view_on_key_up):
> +        (_ewk_view_priv_new):
> +        (_ewk_view_smart_add):
> +        (_ewk_view_smart_zoom_set):
> +        (_ewk_view_zoom_animator_cb):
> +        (_ewk_view_viewport_attributes_compute):
> +        (ewk_view_zoom_set):
> +        (ewk_view_zoom_weak_set):
> +        (ewk_view_zoom_animated_set):
> +        (ewk_view_paint_context_new):
> +        (ewk_view_popup_new):
> +        (ewk_view_popup_destroy):
> +        * ewk/ewk_view_single.cpp:
> +        (_ewk_view_single_smart_add):
> +        (_ewk_view_single_smart_resize):
> +        (_ewk_view_single_scroll_process_single):
> +        (_ewk_view_single_smart_repaints_process):
> +        * ewk/ewk_view_tiled.cpp:
> +        (_ewk_view_tiled_render_cb):
> +        (_ewk_view_tiled_updates_process_pre):
> +        (_ewk_view_tiled_contents_size_changed_cb):
> +        (_ewk_view_tiled_smart_add):

As mentioned by Antonio Gomes in the other giant patch, please remove this from ChangeLog since it's not adding any useful information.

> Source/WebKit/efl/ewk/ewk_view.cpp:1564
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom));

I'm surprised to see we are casting a float to a double. This is a nice time to remove that.

> Source/WebKit/efl/ewk/ewk_view.cpp:1568
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1608
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1612
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1664
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1668
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom));

ditto.

[Gyuyoung Kim]

Created attachment 108604 [details]
Patch

[Gyuyoung Kim]

This is nice time to fix wrong casting. I fixed them. And, meaningless comments are removed from ChangeLog as well.

[Lucas De Marchi]

Comment on attachment 108604 [details]
Patch

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

Please, check the unneeded casts. Especially the float -> double conversions.

> Source/WebKit/efl/ewk/ewk_view.cpp:929
> -        px = (double)(x + cx) / (w + sd->view.w);
> +        px = static_cast<double>((x + cx) / (w + sd->view.w));

Unneeded cast.

> Source/WebKit/efl/ewk/ewk_view.cpp:934
> -        py = (double)(y + cy) / (h + sd->view.h);
> +        py = static_cast<double>((y + cy) / (h + sd->view.h));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1052
> -    int available_width = (int) priv->page->chrome()->client()->pageRect().width();
> -    int available_height = (int) priv->page->chrome()->client()->pageRect().height();
> +    int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width());
> +    int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height());

/me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get().

About the casts, we are probably better off using enclosingIntRect() instead.

> Source/WebKit/efl/ewk/ewk_view.cpp:1055
> -    int device_width = (int) priv->page->chrome()->client()->windowRect().width();
> -    int device_height = (int) priv->page->chrome()->client()->windowRect().height();
> +    int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width());
> +    int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height());

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1564
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1568
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1608
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1612
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1664
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1668
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom));

Unneeded casts.

[Gyuyoung Kim]

>> /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get().

>> About the casts, we are probably better off using enclosingIntRect() instead.

I think that is modified by new bug. So, I think it is better to use static_cast<..> for this bug as below,

1051     int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width());
1052     int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height());
1053 
1054     int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width());
1055     int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height());

Do you think that is fixed first ?

[Lucas De Marchi]

(In reply to comment #11)
> >> /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get().
> 
> >> About the casts, we are probably better off using enclosingIntRect() instead.
> 
> I think that is modified by new bug. So, I think it is better to use static_cast<..> for this bug as below,
> 
> 1051     int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width());
> 1052     int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height());
> 1053 
> 1054     int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width());
> 1055     int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height());
> 

ok.

[Gyuyoung Kim]

Created attachment 108962 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 108962 [details]
Patch

Attachment 108962 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9881573

[Gyuyoung Kim]

Created attachment 108967 [details]
Patch

[Gyuyoung Kim]

Hello Lucas,

My though is a little changed. It seems it is better to remove static_cast<...> of _ewk_view_viewport_attributes_compute() in this bug.

WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
int available_width = available_rect.width();
int available_height = available_rect.height();

WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect());
int device_width = device_rect.width();
int device_height = device_rect.height();

Of course, we can file new bug to decide which functions are used instead of pageRect(), windowRect().

[Lucas De Marchi]

Comment on attachment 108967 [details]
Patch

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

Otherwise looks good now.

> Source/WebKit/efl/ewk/ewk_view.cpp:1053
> -    int available_width = (int) priv->page->chrome()->client()->pageRect().width();
> -    int available_height = (int) priv->page->chrome()->client()->pageRect().height();
> +    WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
> +    int available_width = available_rect.width();
> +    int available_height = available_rect.height();

WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect());

and you don't need available_{width,height}. Just use .width()/.height() below.

> Source/WebKit/efl/ewk/ewk_view.cpp:1057
> -    int device_width = (int) priv->page->chrome()->client()->windowRect().width();
> -    int device_height = (int) priv->page->chrome()->client()->windowRect().height();
> +    WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect());
> +    int device_width = device_rect.width();
> +    int device_height = device_rect.height();

Same here. deviceRect would be more WebKit-style aligned.

[Gyuyoung Kim]

(In reply to comment #17)
> (From update of attachment 108967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review
> 
> Otherwise looks good now.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1053
> > -    int available_width = (int) priv->page->chrome()->client()->pageRect().width();
> > -    int available_height = (int) priv->page->chrome()->client()->pageRect().height();
> > +    WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
> > +    int available_width = available_rect.width();
> > +    int available_height = available_rect.height();
> 
> WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
> 
> and you don't need available_{width,height}. Just use .width()/.height() below.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1057
> > -    int device_width = (int) priv->page->chrome()->client()->windowRect().width();
> > -    int device_height = (int) priv->page->chrome()->client()->windowRect().height();
> > +    WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect());
> > +    int device_width = device_rect.width();
> > +    int device_height = device_rect.height();
> 
> Same here. deviceRect would be more WebKit-style aligned.

efl port is using mixed efl-style and webkit-style for local variable. I think we should make a rule for local variable. This is an new item for Bug 68321. Do you want to remove "_" in available_rect and device_rect in this bug ?

[Lucas De Marchi]

Comment on attachment 108967 [details]
Patch

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

>>> Source/WebKit/efl/ewk/ewk_view.cpp:1053
>>> +    int available_height = available_rect.height();
>> 
>> WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
>> 
>> and you don't need available_{width,height}. Just use .width()/.height() below.
> 
> efl port is using mixed efl-style and webkit-style for local variable. I think we should make a rule for local variable. This is an new item for Bug 68321. Do you want to remove "_" in available_rect and device_rect in this bug ?

I don't care too much about the name now (since it can be made in another bug). Just send another patch removing the useless available_width, available_height, device_width and device_height variables. Then you have my informal r+.

[Gyuyoung Kim]

Created attachment 109168 [details]
Patch

[Gyuyoung Kim]

I remove useless local variables in _ewk_view_viewport_attributes_compute() according to your comment.

[Lucas De Marchi]

Comment on attachment 109168 [details]
Patch

Looks good. Informal r+

[Martin Robinson]

Comment on attachment 109168 [details]
Patch

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

Nice cleanup, but I have some concerns.

> Source/WebKit/efl/ewk/ewk_view.cpp:930
> -        px = (double)(x + cx) / (w + sd->view.w);
> +        px = (x + cx) / (w + sd->view.w);
>      else

Are you sure a cast isn't required here to force floating point arithmetic?

> Source/WebKit/efl/ewk/ewk_view.cpp:934
> -        py = (double)(y + cy) / (h + sd->view.h);
> +        py = (y + cy) / (h + sd->view.h);

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1055
> +    WebCore::IntSize available_size = WebCore::IntSize(available_rect.width(), available_rect.height());
> +    WebCore::ViewportAttributes attributes = WebCore::computeViewportAttributes(priv->viewport_arguments, desktop_width, device_rect.width(), device_rect.height(), device_dpi, available_size);

You should just do available_rect.size()

> Source/WebKit/efl/ewk/ewk_view.cpp:1565
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom);
>          return EINA_FALSE;
>      }
>      if (zoom > priv->settings.zoom_range.max_scale) {
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom);

Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast?

[Lucas De Marchi]

(In reply to comment #23)
> (From update of attachment 109168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review
> 
> Nice cleanup, but I have some concerns.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:930
> > -        px = (double)(x + cx) / (w + sd->view.w);
> > +        px = (x + cx) / (w + sd->view.w);
> >      else
> 
> Are you sure a cast isn't required here to force floating point arithmetic?

This was my bad. I didn't pay attention w and sd->view.w are of type Evas_Coord and not double.
 
 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1565
> > -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> > +        WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom);
> >          return EINA_FALSE;
> >      }
> >      if (zoom > priv->settings.zoom_range.max_scale) {
> > -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> > +        WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom);
> 
> Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast?

These values are floats. This is the reason the cast is not needed.

[Gyuyoung Kim]

(In reply to comment #23)
> (From update of attachment 109168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review
> 
> Nice cleanup, but I have some concerns.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:930
> > -        px = (double)(x + cx) / (w + sd->view.w);
> > +        px = (x + cx) / (w + sd->view.w);
> >      else
> 
> Are you sure a cast isn't required here to force floating point arithmetic?

I think it is better to use a cast explicitly. Frankly I am not sure.

> > Source/WebKit/efl/ewk/ewk_view.cpp:934
> > -        py = (double)(y + cy) / (h + sd->view.h);
> > +        py = (y + cy) / (h + sd->view.h);
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1055
> > +    WebCore::IntSize available_size = WebCore::IntSize(available_rect.width(), available_rect.height());
> > +    WebCore::ViewportAttributes attributes = WebCore::computeViewportAttributes(priv->viewport_arguments, desktop_width, device_rect.width(), device_rect.height(), device_dpi, available_size);
> 
> You should just do available_rect.size()
I missed to use .size(). I will fix it.

> > Source/WebKit/efl/ewk/ewk_view.cpp:1565
> > -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> > +        WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom);
> >          return EINA_FALSE;
> >      }
> >      if (zoom > priv->settings.zoom_range.max_scale) {
> > -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> > +        WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom);
> 
> Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast?

max_scale and zoom variables are float type. So, IMHO, it is better to use "%f" format specifier without casting.

[Gyuyoung Kim]

Created attachment 109226 [details]
Patch

[Martin Robinson]

(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 109168 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review
> > 
> > Nice cleanup, but I have some concerns.
> > 
> > > Source/WebKit/efl/ewk/ewk_view.cpp:930
> > > -        px = (double)(x + cx) / (w + sd->view.w);
> > > +        px = (x + cx) / (w + sd->view.w);
> > >      else
> > 
> > Are you sure a cast isn't required here to force floating point arithmetic?
> 
> I think it is better to use a cast explicitly. Frankly I am not sure.

If Evas_Coord is an integer type then I believe it's necessary.

[Martin Robinson]

Comment on attachment 109226 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:929
> +        px = static_cast<double>((x + cx) / (w + sd->view.w));

Casting the entire expression here is semantically identical to the implicit cast. You need to at least cast the first operand of the division.

[Gyuyoung Kim]

Created attachment 109229 [details]
Patch

[Martin Robinson]

Comment on attachment 109229 [details]
Patch

Nice cleanup!

[WebKit Review Bot]

Comment on attachment 109229 [details]
Patch

Clearing flags on attachment: 109229

Committed r96382: <http://trac.webkit.org/changeset/96382>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.