Re: [PATCH v2 2/2] MdeModulePkg/Browser: Share default if some default value are not specified

["Gao, Liming" <liming.gao@intel.com>]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Bi, Dandan
> Sent: Tuesday, August 09, 2016 9:25 AM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH v2 2/2] MdeModulePkg/Browser: Share default if some
> default value are not specified
> 
> Add a new implementation policy of getting default value in SetupBrowser.
> The new policy is only for the situation that a question has default
> value but doesn't have default value for all supported default type.
> In this case, we will choose the smallest default id from the existing
> defaults, and share its value to other default id which has no
> default value.
> 
> Notes:
> v1->V2:
> Generate DefaultStoreList as ascending order directly instead
> of using function GetDefaultIdArray().
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c | 25
> +++++++++++++++++++----
>  MdeModulePkg/Universal/SetupBrowserDxe/Setup.c    | 23
> ++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c
> b/MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c
> index 11a8fdc..61ba0b5 100644
> --- a/MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c
> +++ b/MdeModulePkg/Universal/SetupBrowserDxe/IfrParse.c
> @@ -1,9 +1,9 @@
>  /** @file
>  Parser for IFR binary encoding.
> 
> -Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php
> 
> @@ -1309,10 +1309,13 @@ ParseOpCodes (
>    EFI_VARSTORE_ID         TempVarstoreId;
>    BOOLEAN                 InScopeDisable;
>    INTN                    ConditionalExprCount;
>    BOOLEAN                 InUnknownScope;
>    UINT8                   UnknownDepth;
> +  FORMSET_DEFAULTSTORE    *PreDefaultStore;
> +  LIST_ENTRY              *DefaultLink;
> +  BOOLEAN                 HaveInserted;
> 
>    SuppressForQuestion      = FALSE;
>    SuppressForOption        = FALSE;
>    InScopeDisable           = FALSE;
>    DepthOfDisable           = 0;
> @@ -1873,21 +1876,35 @@ ParseOpCodes (
> 
>      //
>      // DefaultStore
>      //
>      case EFI_IFR_DEFAULTSTORE_OP:
> +      HaveInserted = FALSE;
>        DefaultStore = AllocateZeroPool (sizeof (FORMSET_DEFAULTSTORE));
>        ASSERT (DefaultStore != NULL);
>        DefaultStore->Signature = FORMSET_DEFAULTSTORE_SIGNATURE;
> 
>        CopyMem (&DefaultStore->DefaultId,   &((EFI_IFR_DEFAULTSTORE *)
> OpCodeData)->DefaultId,   sizeof (UINT16));
>        CopyMem (&DefaultStore->DefaultName, &((EFI_IFR_DEFAULTSTORE *)
> OpCodeData)->DefaultName, sizeof (EFI_STRING_ID));
> -
>        //
> -      // Insert to DefaultStore list of this Formset
> +      // Insert it to the DefaultStore list of this Formset with ascending order.
>        //
> -      InsertTailList (&FormSet->DefaultStoreListHead, &DefaultStore->Link);
> +      if (!IsListEmpty (&FormSet->DefaultStoreListHead)) {
> +        DefaultLink = GetFirstNode (&FormSet->DefaultStoreListHead);
> +        while (!IsNull (&FormSet->DefaultStoreListHead, DefaultLink)) {
> +          PreDefaultStore =
> FORMSET_DEFAULTSTORE_FROM_LINK(DefaultLink);
> +          DefaultLink = GetNextNode (&FormSet->DefaultStoreListHead,
> DefaultLink);
> +          if (DefaultStore->DefaultId < PreDefaultStore->DefaultId) {
> +            InsertTailList (&PreDefaultStore->Link, &DefaultStore->Link);
> +            HaveInserted = TRUE;
> +            break;
> +          }
> +        }
> +      }
> +      if (!HaveInserted) {
> +        InsertTailList (&FormSet->DefaultStoreListHead, &DefaultStore->Link);
> +      }
>        break;
> 
>      //
>      // Statements
>      //
> diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> index 6b38547..66c4313 100644
> --- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> +++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> @@ -4048,13 +4048,18 @@ GetQuestionDefault (
>    EFI_HII_CONFIG_ACCESS_PROTOCOL  *ConfigAccess;
>    EFI_BROWSER_ACTION_REQUEST      ActionRequest;
>    INTN                            Action;
>    CHAR16                          *NewString;
>    EFI_IFR_TYPE_VALUE              *TypeValue;
> +  UINT16                          OriginalDefaultId;
> +  FORMSET_DEFAULTSTORE            *DefaultStore;
> +  LIST_ENTRY                      *DefaultLink;
> 
>    Status   = EFI_NOT_FOUND;
>    StrValue = NULL;
> +  OriginalDefaultId  = DefaultId;
> +  DefaultLink        = GetFirstNode (&FormSet->DefaultStoreListHead);
> 
>    //
>    // Statement don't have storage, skip them
>    //
>    if (Question->QuestionId == 0) {
> @@ -4067,10 +4072,11 @@ GetQuestionDefault (
>    //  2, use ExtractConfig function
>    //  3, use nested EFI_IFR_DEFAULT
>    //  4, set flags of EFI_ONE_OF_OPTION (provide Standard and
> Manufacturing default)
>    //  5, set flags of EFI_IFR_CHECKBOX (provide Standard and Manufacturing
> default) (lowest priority)
>    //
> +ReGetDefault:
>    HiiValue = &Question->HiiValue;
>    TypeValue = &HiiValue->Value;
>    if (HiiValue->Type == EFI_IFR_TYPE_BUFFER) {
>      //
>      // For orderedlist, need to pass the BufferValue to Callback function.
> @@ -4233,11 +4239,26 @@ GetQuestionDefault (
>        return EFI_SUCCESS;
>      }
>    }
> 
>    //
> -  // For Questions without default
> +  // For question without default value for current default Id, we try to re-
> get the default value form other default id in the DefaultStoreList.
> +  // If get, will exit the function, if not, will choose next default id in the
> DefaultStoreList.
> +  // The default id in DefaultStoreList are in ascending order to make sure
> choose the smallest default id every time.
> +  //
> +  while (!IsNull(&FormSet->DefaultStoreListHead, DefaultLink)) {
> +    DefaultStore = FORMSET_DEFAULTSTORE_FROM_LINK(DefaultLink);
> +    DefaultLink = GetNextNode (&FormSet-
> >DefaultStoreListHead,DefaultLink);
> +    DefaultId = DefaultStore->DefaultId;
> +    if (DefaultId == OriginalDefaultId) {
> +      continue;
> +    }
> +    goto ReGetDefault;
> +  }
> +
> +  //
> +  // For Questions without default value for all the default id in the
> DefaultStoreList.
>    //
>    Status = EFI_NOT_FOUND;
>    switch (Question->Operand) {
>    case EFI_IFR_NUMERIC_OP:
>      //
> --
> 1.9.5.msysgit.1