ash: CalendarMonthView fetches events

Instead of binding calendar event fetching to scrolling of
CalendarView, bind it to construction of individual CalendarMonthView
instances. CalendarMonthView's constructor fetches events, the view is
re-painted when events arrive in OnEventsFetched(), and (if necessary)
the fetch is cancelled from CalendarMonthView's destructor. Reasons
for doing this:

(1) More efficient redraw, since a CalendarMonthView only repaints its
children if the loading status is "success" and the events it receives
are from its own month.

(2) Allows CalendarMonthView to (eventually) assume control of
put-up/tear-down of the "loading..." indicator.

(3) Eliminates the need to maintain a sliding window of visible months
to be fetched.

CalendarEventFetch now uses calendar_utils::GetFetchStartEndTimes() to
get fetch start/end times adjusted for time zone differences and
daylight savings.

Bug: 1298357
Change-Id: I02aaca184f9bd6206c4f1f83b32ec787544a8a35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3564628
Reviewed-by: Jiaming Cheng <jiamingc@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Commit-Queue: Roger Tinkoff <rtinkoff@google.com>
Cr-Commit-Position: refs/heads/main@{#1002602}

ash/system/time/calendar_event_fetch.cc

@@ -32,15 +32,19 @@ CalendarEventFetch::CalendarEventFetch(
   CalendarClient* client = Shell::Get()->calendar_controller()->GetClient();
   DCHECK(client);
 
-  const base::Time start_of_next_month =
-      calendar_utils::GetStartOfNextMonthUTC(start_of_month);
+  const std::pair<base::Time, base::Time> fetch_times =
+      calendar_utils::GetFetchStartEndTimes(start_of_month);
 
-  if (ash::features::IsCalendarModelDebugModeEnabled())
-    VLOG(1) << "Fetching: " << start_of_month << " => " << start_of_next_month;
+  if (ash::features::IsCalendarModelDebugModeEnabled()) {
+    VLOG(1) << "Fetching: " << fetch_times.first << " => "
+            << fetch_times.second;
+  }
 
-  client->GetEventList(base::BindOnce(&CalendarEventFetch::OnResultReceived,
-                                      weak_factory_.GetWeakPtr()),
-                       start_of_month, start_of_next_month);
+  cancel_closure_ =
+      client->GetEventList(base::BindOnce(&CalendarEventFetch::OnResultReceived,
+                                          weak_factory_.GetWeakPtr()),
+                           fetch_times.first, fetch_times.second);
+  DCHECK(cancel_closure_);
 
   timeout_.Start(
       FROM_HERE, calendar_utils::kEventFetchTimeout,
@@ -49,6 +53,10 @@ CalendarEventFetch::CalendarEventFetch(
 
 CalendarEventFetch::~CalendarEventFetch() = default;
 
+void CalendarEventFetch::Cancel() {
+  std::move(cancel_closure_).Run();
+}
+
 void CalendarEventFetch::OnResultReceived(
     google_apis::ApiErrorCode error,
     std::unique_ptr<google_apis::calendar::EventList> events) {

ash/system/time/calendar_event_fetch.h

@@ -44,6 +44,9 @@ class ASH_EXPORT CalendarEventFetch {
   CalendarEventFetch& operator=(const CalendarEventFetch& other) = delete;
   ~CalendarEventFetch();
 
+  // Cancels the fetch request, invokes `cancel_closure_`.
+  void Cancel();
+
  private:
   // Callback invoked when results of a fetch are available.
   void OnResultReceived(
@@ -69,6 +72,9 @@ class ASH_EXPORT CalendarEventFetch {
   // go too long without a response.
   base::OneShotTimer timeout_;
 
+  // Closure to be invoked if the request needs to be canceled.
+  base::OnceClosure cancel_closure_;
+
   base::WeakPtrFactory<CalendarEventFetch> weak_factory_{this};
 };
 

ash/system/time/calendar_event_fetch_unittest.cc

@@ -51,9 +51,12 @@ class TestCalendarClient : public CalendarClient {
     // like a event fetch in production.  Use set_response_delay() to use a
     // value that's different from the default.
     StartResponseDelayTimeout();
-    return base::DoNothing();
+    return base::BindOnce(&TestCalendarClient::CancelCallback,
+                          weak_factory_.GetWeakPtr());
   }
 
+  void CancelCallback() { set_api_error_code(google_apis::CANCELLED); }
+
   void set_event_list(
       std::unique_ptr<google_apis::calendar::EventList> event_list) {
     event_list_ = std::move(event_list);
@@ -362,4 +365,29 @@ TEST_F(CalendarEventFetchTest, Timeout) {
                                      /*expected_count=*/1);
 }
 
+TEST_F(CalendarEventFetchTest, Cancel) {
+  // Register our TestCalendarClient with the default user.
+  RegisterClient();
+
+  // Month whose events we want to fetch.
+  base::Time start_of_month =
+      GetStartOfMonthFromString("23 Oct 2009 11:30 GMT");
+
+  // Perform the fetch.
+  std::unique_ptr<CalendarEventFetch> fetch = PerformFetch(start_of_month);
+
+  // Cancel the request.
+  fetch->Cancel();
+
+  // Advance time to when the fetch is complete. `fetch` can no longer be used
+  // after this.
+  task_environment()->FastForwardBy(client_.get_response_delay());
+
+  // API error is CANCELLED.
+  absl::optional<google_apis::ApiErrorCode> return_error_code =
+      api_error_code();
+  EXPECT_TRUE(return_error_code.has_value() &&
+              return_error_code == google_apis::CANCELLED);
+}
+
 }  // namespace ash

ash/system/time/calendar_model.cc

@@ -97,8 +97,8 @@ void CalendarModel::MaybeFetchMonth(base::Time start_of_month) {
   if (!calendar_utils::IsActiveUser())
     return;
 
-  // Bail out early if we have no CalendarClient.  This will be the case in most
-  // unit tests.
+  // Bail out early if there is no CalendarClient.  This will be the case in
+  // most unit tests.
   CalendarClient* client = Shell::Get()->calendar_controller()->GetClient();
   if (!client)
     return;
@@ -165,7 +165,7 @@ void CalendarModel::ClearAllCachedEvents() {
   // Destroy all outstanding fetch requests.
   pending_fetches_.clear();
 
-  // Destroy the set of months we've fetched.
+  // Destroy the set of months that have been fetched.
   months_fetched_.clear();
 
   // Destroy all prunable months.
@@ -219,14 +219,24 @@ void CalendarModel::FetchEventsSurrounding(int num_months,
   FetchEvents(months);
 }
 
+void CalendarModel::CancelFetch(const base::Time& start_of_month) {
+  if (base::Contains(pending_fetches_, start_of_month)) {
+    // Note that the `CalendarEventFetch` here will be removed from
+    // `pending_fetches_` in `OnEventsFetched`, which will receive an error code
+    // of `google_apis::CANCELLED` and an empty event list, so there's no need
+    // to remove it here.
+    pending_fetches_[start_of_month]->Cancel();
+  }
+}
+
 int CalendarModel::EventsNumberOfDayInternal(base::Time day,
                                              SingleDayEventList* events) const {
   const SingleDayEventList& list = FindEvents(day);
 
   if (list.empty())
     return 0;
 
-  // We have events, and we assume the destination is empty.
+  // There are events, and the destination should be empty.
   if (events) {
     DCHECK(events->empty());
     *events = list;
@@ -249,13 +259,11 @@ void CalendarModel::OnEventsFetched(
     const google_apis::calendar::EventList* events) {
   base::UmaHistogramSparse("Ash.Calendar.FetchEvents.Result", error);
   if (error != google_apis::HTTP_SUCCESS) {
-    LOG(ERROR) << __FUNCTION__ << " Event fetch received error: " << error;
     // Request is no longer outstanding, so it can be destroyed.
-    // TODO: https://crbug.com/1298187 We need to respond further based on the
-    // specific error code, retry in some cases, etc.
     pending_fetches_.erase(start_of_month);
-    // TODO: https://crbug.com/1298187 maybe notify observers.
-    // e.g. observer.OnEventsFetched(kError, start_of_month, events);
+    // TODO(https://crbug.com/1298187): Possibly respond further based on the
+    // specific error code, retry in some cases, etc. Or notify observers e.g.
+    // observer.OnEventsFetched(kError, start_of_month, events);
     return;
   }
 
@@ -297,7 +305,7 @@ void CalendarModel::OnEventsFetched(
   event_months_.erase(start_of_month);
 
   if (!events || events->items().empty()) {
-    // Even though `start_of_month` has no events, we insert an empty map to
+    // Even though `start_of_month` has no events, insert an empty map to
     // indicate a successful fetch.
     SingleMonthEventMap empty_event_map;
     event_months_.emplace(start_of_month, empty_event_map);
@@ -322,20 +330,17 @@ void CalendarModel::OnEventsFetched(
                              GetEventMapSize(event_months_[start_of_month]));
 
   // If `start_of_month` is further, in months, from the on-screen month when
-  // the calendar first opened, then update our max distance.
+  // the calendar first opened, then update the max distance.
   UpdateMaxDistanceBrowsed(start_of_month);
 }
 
 void CalendarModel::OnEventFetchFailedInternalError(
     base::Time start_of_month,
     CalendarEventFetchInternalErrorCode error) {
-  LOG(ERROR) << __FUNCTION__
-             << " Event fetch received internal error: " << (int)error;
-
   // Request is no longer outstanding, so it can be destroyed.
-  // TODO: https://crbug.com/1298187 We need to respond further based on the
-  // specific error code, retry in some cases, etc.
   pending_fetches_.erase(start_of_month);
+  // TODO(https://crbug.com/1298187): May need to respond further based on the
+  // specific error code, retry in some cases, etc.
 }
 
 void CalendarModel::UpdateMaxDistanceBrowsed(const base::Time& start_of_month) {
@@ -457,13 +462,13 @@ void CalendarModel::InsertEventsForTesting(
 SingleDayEventList CalendarModel::FindEvents(base::Time day) const {
   SingleDayEventList event_list;
 
-  // Early return if we know we have no events for this month.
+  // Early return if there are no events for this month.
   base::Time start_of_month = calendar_utils::GetStartOfMonthUTC(day);
   auto it = event_months_.find(start_of_month);
   if (it == event_months_.end())
     return event_list;
 
-  // Early return if we know we have no events for this day.
+  // Early return if there are no events for this day.
   base::Time midnight = day.UTCMidnight();
   const SingleMonthEventMap& month = it->second;
   auto it2 = month.find(midnight);

ash/system/time/calendar_model.h

@@ -92,6 +92,9 @@ class ASH_EXPORT CalendarModel : public SessionObserver {
   // Requests events that fall in `num_months` months surrounding `day`.
   void FetchEventsSurrounding(int num_months, const base::Time day);
 
+  // Cancels any pending event fetch for `start_of_month`.
+  void CancelFetch(const base::Time& start_of_month);
+
   // Same as `FindEvents`, except that return of any events on `day` constitutes
   // "use" in the most-recently-used sense, so the month that includes day will
   // then be promoted to most-recently-used status.  Use this to get events if

ash/system/time/calendar_month_view.cc

@@ -10,6 +10,7 @@
 #include "ash/style/ash_color_provider.h"
 #include "ash/system/model/system_tray_model.h"
 #include "ash/system/time/calendar_metrics.h"
+#include "ash/system/time/calendar_model.h"
 #include "ash/system/time/calendar_utils.h"
 #include "ash/system/time/calendar_view_controller.h"
 #include "base/bind.h"
@@ -42,7 +43,7 @@ constexpr float kTodayRoundedRadius = 22.f;
 // The radius used to draw rounded today's circle when focused.
 constexpr float kTodayFocusedRoundedRadius = 18.f;
 
-// Radius of the small dot we display on a CalendarDateCellView if events are
+// Radius of the small dot displayed on a CalendarDateCellView if events are
 // present for that day.
 constexpr float kEventsPresentRoundedRadius = 1.f;
 
@@ -116,8 +117,8 @@ void CalendarDateCellView::OnThemeChanged() {
 
 // Draws the background for this date. Note that this includes not only the
 // circular fill (if any), but also the border (if focused) and text color. If
-// this is a grayed out date, which is shown in its previous/next month, we
-// won't draw this background.
+// this is a grayed out date, which is shown in its previous/next month, this
+// background won't be drawn.
 void CalendarDateCellView::OnPaintBackground(gfx::Canvas* canvas) {
   if (grayed_out_)
     return;
@@ -315,7 +316,8 @@ void CalendarDateCellView::OnDateCellActivated(const ui::Event& event) {
 CalendarMonthView::CalendarMonthView(
     const base::Time first_day_of_month,
     CalendarViewController* calendar_view_controller)
-    : calendar_view_controller_(calendar_view_controller) {
+    : calendar_view_controller_(calendar_view_controller),
+      calendar_model_(Shell::Get()->system_tray_model()->calendar_model()) {
   views::TableLayout* layout =
       SetLayoutManager(std::make_unique<views::TableLayout>());
   // The layer is required in animation.
@@ -341,6 +343,10 @@ CalendarMonthView::CalendarMonthView(
   base::Time::Exploded current_date_exploded =
       calendar_utils::GetExplodedUTC(current_date_local);
 
+  // Fetch events for the month.
+  fetch_month_ = first_day_of_month_local.UTCMidnight();
+  FetchEvents(fetch_month_);
+
   // TODO(https://crbug.com/1236276): Extract the following 3 parts (while
   // loops) into a method.
   int column = 0;
@@ -382,6 +388,9 @@ CalendarMonthView::CalendarMonthView(
 
   last_row_index_ = row_number - 1;
 
+  // To receive the fetched events.
+  scoped_calendar_model_observer_.Observe(calendar_model_);
+
   if (calendar_utils::GetDayOfWeek(current_date) ==
       calendar_utils::kFirstDayOfWeekString)
     return;
@@ -409,7 +418,21 @@ CalendarMonthView::CalendarMonthView(
   }
 }
 
-CalendarMonthView::~CalendarMonthView() = default;
+CalendarMonthView::~CalendarMonthView() {
+  calendar_model_->CancelFetch(fetch_month_);
+}
+
+void CalendarMonthView::FetchEvents(const base::Time& month) {
+  calendar_model_->FetchEvents({month});
+}
+
+void CalendarMonthView::OnEventsFetched(
+    const CalendarModel::FetchingStatus status,
+    const base::Time start_time,
+    const google_apis::calendar::EventList* events) {
+  if (status == CalendarModel::kSuccess && start_time == fetch_month_)
+    SchedulePaintChildren();
+}
 
 CalendarDateCellView* CalendarMonthView::AddDateCellToLayout(
     base::Time current_date,

ash/system/time/calendar_month_view.h

@@ -6,6 +6,7 @@
 #define ASH_SYSTEM_TIME_CALENDAR_MONTH_VIEW_H_
 
 #include "ash/ash_export.h"
+#include "ash/system/time/calendar_model.h"
 #include "ash/system/time/calendar_view_controller.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
@@ -37,7 +38,7 @@ class CalendarDateCellView : public CalendarViewController::Observer,
   void OnThemeChanged() override;
 
   // Draws the background for 'today'. If today is a grayed out date, which is
-  // shown in its previous/next month, we won't draw this background.
+  // shown in its previous/next month, this background won't be drawn.
   void OnPaintBackground(gfx::Canvas* canvas) override;
 
   // CalendarViewController::Observer:
@@ -74,10 +75,10 @@ class CalendarDateCellView : public CalendarViewController::Observer,
   // Callback called when this view is activated.
   void OnDateCellActivated(const ui::Event& event);
 
-  // Computes the position of the indicator that our day has events.
+  // Computes the position of the indicator that the day has events.
   gfx::Point GetEventsPresentIndicatorCenterPosition();
 
-  // Draw the indicator if our day has events.
+  // Draw the indicator if the day has events.
   void MaybeDrawEventsIndicator(gfx::Canvas* canvas);
 
   // The date used to render this cell view.
@@ -107,14 +108,20 @@ class CalendarDateCellView : public CalendarViewController::Observer,
 };
 
 //  Container for `CalendarDateCellView` for a single month.
-class ASH_EXPORT CalendarMonthView : public views::View {
+class ASH_EXPORT CalendarMonthView : public views::View,
+                                     public CalendarModel::Observer {
  public:
   CalendarMonthView(base::Time first_day_of_month,
                     CalendarViewController* calendar_view_controller);
   CalendarMonthView(const CalendarMonthView& other) = delete;
   CalendarMonthView& operator=(const CalendarMonthView& other) = delete;
   ~CalendarMonthView() override;
 
+  // CalendarModel::Observer:
+  void OnEventsFetched(const CalendarModel::FetchingStatus status,
+                       const base::Time start_time,
+                       const google_apis::calendar::EventList* events) override;
+
   // Enable each cell's focus behavior.
   void EnableFocus();
 
@@ -141,6 +148,9 @@ class ASH_EXPORT CalendarMonthView : public views::View {
                                             bool is_in_current_month,
                                             int row_index);
 
+  // Fetches events.
+  void FetchEvents(const base::Time& month);
+
   // Owned by `CalendarView`.
   CalendarViewController* const calendar_view_controller_;
 
@@ -153,6 +163,16 @@ class ASH_EXPORT CalendarMonthView : public views::View {
   // The cells of each row that should be first focused on. These
   // `CalendarDateCellView`s are the children of this view.
   std::vector<CalendarDateCellView*> focused_cells_;
+
+  // UTC midnight to designate the month whose events will be fetched.
+  base::Time fetch_month_;
+
+  // Raw pointer to the (singleton) CalendarModel, to avoid a bunch of
+  // daisy-chained calls to get the std::unique_ptr<>.
+  CalendarModel* const calendar_model_;
+
+  base::ScopedObservation<CalendarModel, CalendarModel::Observer>
+      scoped_calendar_model_observer_{this};
 };
 
 }  // namespace ash