New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 849614 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Team-Accessibility



Sign in to add a comment

a11y: Possible misleading indentation in ax_menu_list_popup.cc

Project Member Reported by raphael....@intel.com, Jun 5 2018

Issue description

I was recently doing a Chromium build with GCC, which threw a -Wmisleading-indentation warning when compiling third_party/blink/renderer/modules/accessibility/ax_menu_list_popup.cc:

  void AXMenuListPopup::DidHide() {
    // ...
    if (ActiveDescendant())
      cache.PostNotification(this, AXObjectCacheImpl::kAXChildrenChanged);
      cache.PostNotification(ActiveDescendant(),
                             AXObjectCacheImpl::kAXMenuListItemUnselected);
  }

The first call to PostNotification() didn't exist until https://chromium-review.googlesource.com/c/chromium/src/+/769674, and now the second one is made even if the if clause doesn't evaluate to true. Since I'm not familiar with the code, I'm filing the bug so that someone else can decide whether the the code just needs braces or if things are working as intended and the code needs to be reindented.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a628d4ff6707f89c1b8300a0d9e31eb06de0654c

commit a628d4ff6707f89c1b8300a0d9e31eb06de0654c
Author: DongJun Kim <djmix.kim@samsung.com>
Date: Wed Jun 13 08:01:22 2018

Adding missing braces to AXMenuListPopup::DidHide()

Chromium build with GCC, it had -Wmisleading-indentation
warning in ax_menu_list_popup.cc after commit [1].

It looks unintended and we adds missing braces from
previous patch-set.

Also this patch includes clean-up as removing
redundant function calls.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/769674

Bug:  849614 
Change-Id: I45051847e116bc055dd8ba954bd9a0730d7188df
Reviewed-on: https://chromium-review.googlesource.com/1090698
Commit-Queue: DongJun Kim <djmix.kim@samsung.com>
Reviewed-by: Katie Dektar <katie@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566755}
[modify] https://crrev.com/a628d4ff6707f89c1b8300a0d9e31eb06de0654c/third_party/blink/renderer/modules/accessibility/ax_menu_list_popup.cc

Cc: djmix....@samsung.com
Labels: M-69
Status: Fixed (was: Available)
Thanks for the patch!

Sign in to add a comment